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.

Sign up for GitHub

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

Jump to bottom

Epoll type #1882

Merged
merged 1 commit into from
Dec 6, 2022
Merged

Epoll type #1882

merged 1 commit into from
Dec 6, 2022

Conversation

JonathanWoollett-Light
Copy link
Contributor

Epoll can be most safely used as a type. This implement a type Epoll which supports this.

asomers
asomers requested changes Nov 28, 2022
View reviewed changes
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

This looks like a good improvement. But I think you should deprecate the old interface. We don't want to provide two APIs for doing the same thing. Also, it needs a CHANGELOG entry.

src/sys/epoll.rs Show resolved Hide resolved
src/sys/epoll.rs
/// # use nix::unistd::write;
/// # use std::os::unix::io::{OwnedFd, FromRawFd, AsRawFd, AsFd};
/// # use std::time::{Instant, Duration};
/// # fn main() -> nix::Result<()> {
Copy link
Member

Choose a reason for hiding this comment

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

Do you need to declare the main() function explicitly? If you leave it out, I think rustdoc will add it automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rustdoc automatically adds main() {} without a return type. To use ? requires manually writing it with a return type.

src/sys/epoll.rs Outdated Show resolved Hide resolved
@JonathanWoollett-Light
Copy link
Contributor Author

This looks like a good improvement. But I think you should deprecate the old interface. We don't want to provide two APIs for doing the same thing. Also, it needs a CHANGELOG entry.

Is there a policy on deprecation possibly using #[deprecated]? Or can I just remove the old implementations?

@asomers
Copy link
Member

asomers commented Nov 29, 2022

Is there a policy on deprecation possibly using #[deprecated]? Or can I just remove the old implementations?

Yes, you need to #[deprecated] them for at least one release before removing them.

@asomers asomers added this to the 0.27.0 milestone Nov 29, 2022
@JonathanWoollett-Light
Copy link
Contributor Author

Is there a policy on deprecation possibly using #[deprecated]? Or can I just remove the old implementations?

Yes, you need to #[deprecated] them for at least one release before removing them.

Added #[deprecated] am I correct in adding since = 0.26.0? (since the function will be deprecated in 0.26.0)

@asomers
Copy link
Member

asomers commented Nov 29, 2022

Added #[deprecated] am I correct in adding since = 0.26.0? (since the function will be deprecated in 0.26.0)

No. Due to the MSRV bump, release 0.26.0 won't include any of the I/O Safety stuff. So you should change it to 0.27.0.

@JonathanWoollett-Light
Copy link
Contributor Author

@asomers Updated it to 0.27.0

asomers
asomers requested changes Dec 3, 2022
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
src/sys/epoll.rs Outdated Show resolved Hide resolved
src/sys/epoll.rs Outdated
pub fn epoll_ctl<'a, T>(
&self,
op: EpollOp,
fd: RawFd,
Copy link
Member

Choose a reason for hiding this comment

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

In the future, we should probably make this BorrowedFd, but not until that type has had time to trickle through the ecosystem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated it to be Fd where Fd: AsFd matching the other functions like Epoll::add etc. I'm not sure about the case of specifically using BorrowedFd as this prohibits someone simply passing a reference to an OwnedFd and requires fd.as_fd() rather than &fd.

src/sys/epoll.rs
/// instance referred to by `self`. It requests that the operation `op` be performed for the
/// target file descriptor, `fd`.
///
/// When possible prefer [`Epoll::add`], [`Epoll::delete`] and [`Epoll::modify`].
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason to make this public? Any operation that can't be performed with add, delete, or modify?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I'll make it private.

@asomers asomers mentioned this pull request Dec 3, 2022
Open
29 tasks
asomers
asomers requested changes Dec 4, 2022
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
asomers
asomers approved these changes Dec 6, 2022
View reviewed changes
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

bors r+

@bors
Copy link
Contributor

bors bot commented Dec 6, 2022

Build succeeded:

@bors bors bot merged commit 4e0c970 into nix-rust:master Dec 6, 2022
This was referenced Aug 28, 2023
Open
Open
@SteveLauC SteveLauC mentioned this pull request Nov 25, 2023
Merged
3 tasks
@SteveLauC SteveLauC mentioned this pull request Dec 9, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asomers asomers asomers approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.27.0
Development

Successfully merging this pull request may close these issues.
2 participants
@JonathanWoollett-Light @asomers