Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replace legacy impl with Poll #329

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

loongs-zhang
Copy link
Contributor

No description provided.

@ihciah
Copy link
Member

ihciah commented Feb 5, 2025

Thank you very much! Please allow me some time to review it!

@ihciah
Copy link
Member

ihciah commented Feb 26, 2025

Could you re-push it and trigger the CI?
CI was broken before because the company org misconfigured the Action.

@loongs-zhang
Copy link
Contributor Author

You can also saw CI results in https://github.com/loongs-zhang/monoio/commits/dev-refactor-poll
image

@loongs-zhang
Copy link
Contributor Author

I think the CI fails due to cross.

Copy link
Member

@ihciah ihciah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good change which I wanted long time before(#269).
The only thing I concern is the legacy feature. I think it cannot be treated as poll-io, which is a supplement to polling ability under iouring.

@ihciah
Copy link
Member

ihciah commented Feb 27, 2025

And another unrelated issue I think about when reading my old PR draft, the latest monoio-macro's test macro seems not work in user code due to some #cfg you introduced 13 months ago. They are not published so the problem haven't affected users. For this issue, I can think about these cases:

  1. #[monoio::test] in user code: use iouring by default
  2. #[monoio::test(driver=legacy)] in user code: use legacy driver
  3. #[monoio::test_all] in user code: generate tests for both drivers
  4. #[monoio::test_all] in monoio code: generate tests for both drivers with cfg.

Maybe we can come about a way to fix and do it in another PR.

@loongs-zhang loongs-zhang force-pushed the dev-refactor-poll branch 4 times, most recently from 1d38188 to 2e26222 Compare February 28, 2025 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants