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

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.

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
/// # 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

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.

/// 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.

CHANGELOG.md Outdated Show resolved Hide resolved
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+

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