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

Add `Interests` for registering process(Issue #908) #925

Merged
merged 14 commits into from May 6, 2019

Conversation

@FXTi
Copy link
Contributor

FXTi commented Apr 18, 2019

Add new Interests based on mio-st version and Ready struct.

@FXTi FXTi force-pushed the FXTi:issue908 branch from 8c9c88b to 5e81156 Apr 19, 2019
@carllerche carllerche requested a review from Thomasdezeeuw Apr 25, 2019
@carllerche

This comment has been minimized.

Copy link
Member

carllerche commented Apr 25, 2019

@FXTi Thanks for doing this. Could you merge master? This should fix CI.

@Thomasdezeeuw does this look good to you?

Copy link
Collaborator

Thomasdezeeuw left a comment

A rebase is required after the latests changes on master.

It mostly looks good, I've added to some comment but there mostly small things.

src/event_imp.rs Outdated Show resolved Hide resolved
src/event_imp.rs Outdated Show resolved Hide resolved
src/event_imp.rs Outdated Show resolved Hide resolved
src/event_imp.rs Outdated Show resolved Hide resolved
src/event_imp.rs Outdated Show resolved Hide resolved
src/sys/unix/dlsym.rs Outdated Show resolved Hide resolved
src/sys/unix/epoll.rs Outdated Show resolved Hide resolved
src/sys/unix/kqueue.rs Outdated Show resolved Hide resolved
src/sys/unix/ready.rs Outdated Show resolved Hide resolved
src/event_imp.rs Show resolved Hide resolved
@FXTi FXTi force-pushed the FXTi:issue908 branch from 3dcfd33 to 73283d5 Apr 27, 2019
@Thomasdezeeuw

This comment has been minimized.

Copy link
Collaborator

Thomasdezeeuw commented Apr 27, 2019

@FXTi can you rebase on master again, you're currently adding a lot of code that was just removed, e.g. src/channel.rs.

FXTi added 10 commits Apr 18, 2019
Add Interests definition and implementation.
Change the register / reregister apis to interest style
Fix broken test cases for `Interests`.

And add sub operation for `Interests`.
Fix the Windows part and kqueue part under Unix.
* Fix test cases and examples in comments
* Use `UnixReady` to provide additional flags for `Interests`
Add `Interests` readiness for Unix platfrom
@FXTi FXTi force-pushed the FXTi:issue908 branch from 9465cce to 49c8a3a Apr 29, 2019
test/test_write_then_drop.rs Show resolved Hide resolved
test/test_write_then_drop.rs Show resolved Hide resolved
src/event_imp.rs Outdated Show resolved Hide resolved
src/event_imp.rs Outdated Show resolved Hide resolved
src/event_imp.rs Outdated Show resolved Hide resolved
src/event_imp.rs Outdated Show resolved Hide resolved
test/test_echo_server.rs Outdated Show resolved Hide resolved
test/test_register_deregister.rs Outdated Show resolved Hide resolved
test/test_uds_shutdown.rs Outdated Show resolved Hide resolved
test/test_unix_echo_server.rs Outdated Show resolved Hide resolved
@stjepang

This comment has been minimized.

Copy link
Contributor

stjepang commented Apr 29, 2019

Just a really minor nit. I see interest: Interests everywhere, which should probably be changed to interests: Interests.

@FXTi

This comment has been minimized.

Copy link
Contributor Author

FXTi commented May 2, 2019

@carllerche @Thomasdezeeuw Is this ready to go? Or still changes requested.

Copy link
Collaborator

Thomasdezeeuw left a comment

I've added two more comments, otherwise LGTM.

Sorry this is taking so long, but thank you for sticking with it!

///
/// [`Poll`]: struct.Poll.html
#[inline]
pub fn both() -> Interests {

This comment has been minimized.

Copy link
@Thomasdezeeuw

Thomasdezeeuw May 2, 2019

Collaborator

Lets make this private for now and we'll figure out both vs all in future pr.

src/event_imp.rs Outdated Show resolved Hide resolved
@FXTi

This comment has been minimized.

Copy link
Contributor Author

FXTi commented May 2, 2019

I've also learned something, so thank you : )

Copy link
Collaborator

Thomasdezeeuw left a comment

LGTM, @carllerche you want to merge this?

@carllerche carllerche merged commit 631e80a into tokio-rs:master May 6, 2019
15 checks passed
15 checks passed
FreeBSD 11.2 amd64 Task Summary
Details
tokio-rs.mio Build #20190502.1 succeeded
Details
tokio-rs.mio (Check rustfmt) Check rustfmt succeeded
Details
tokio-rs.mio (Cross Android) Cross Android succeeded
Details
tokio-rs.mio (Cross Android_64) Cross Android_64 succeeded
Details
tokio-rs.mio (Cross NetBSD) Cross NetBSD succeeded
Details
tokio-rs.mio (Cross Solaris) Cross Solaris succeeded
Details
tokio-rs.mio (Cross iOS) Cross iOS succeeded
Details
tokio-rs.mio (Min Rust Linux) Min Rust Linux succeeded
Details
tokio-rs.mio (Min Rust MacOS) Min Rust MacOS succeeded
Details
tokio-rs.mio (Min Rust Windows) Min Rust Windows succeeded
Details
tokio-rs.mio (Nightly Linux) Nightly Linux succeeded
Details
tokio-rs.mio (Test Linux) Test Linux succeeded
Details
tokio-rs.mio (Test MacOS) Test MacOS succeeded
Details
tokio-rs.mio (Test Windows) Test Windows succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.