Add select::FdSet::fds() method#1207
Add select::FdSet::fds() method#1207bors[bot] merged 8 commits intonix-rust:masterfrom zombiezen:select-fd-iterator
Conversation
asomers
left a comment
asomers
left a comment
There was a problem hiding this comment.
This looks like a good feature. Let's wait to see if rust-lang/libc#1725 is approved before we decide how to implement contains
| pub fn contains(&mut self, fd: RawFd) -> bool { | ||
| unsafe { libc::FD_ISSET(fd, &mut self.0) } | ||
| pub fn contains(&self, fd: RawFd) -> bool { | ||
| let mut copy = self.0; |
There was a problem hiding this comment.
libc::FD_ISSET should be taking a *const pointer rather than *mut. I just opened a PR to fix that.
rust-lang/libc#1725
There was a problem hiding this comment.
As an aside, if we're worried about the cost of this copy, then we might want to consider not deriving Copy for FdSet.
There was a problem hiding this comment.
You know, that's an excellent point. On my system, FdSet is 128 bytes big. Users probably shouldn't copy such a time unless they really need to. However removing Copy doesn't necessarily prevent the value from ever being memcpy'd, and the consensus is that it's best to impl Copy regardless of size. See https://www.reddit.com/r/rust/comments/2xxjda/when_should_my_type_be_copy/ and https://internals.rust-lang.org/t/pre-rfc-copy-like-trait-for-large-types/11852/14 for some discussion.
There was a problem hiding this comment.
I think that for now we should proceed without waiting for rust-lang/libc#1725 . But copying the entire structure on each loop of the iterator is woefully inefficient. I think you should retain the &mut reference instead.
There was a problem hiding this comment.
Done. This sadly means that the iterator cannot implement Clone, but I don't see a good way around that.
| /// assert_eq!(fds, vec![4, 9]); | ||
| /// ``` | ||
| #[inline] | ||
| pub fn fds(&self) -> Fds { |
There was a problem hiding this comment.
Typically a C programmer will know what the maximum file descriptor he registered was, and when inspecting the fd_set he won't need to iterate all the way to FD_SETSIZE, which can be up to 64k on some platforms. But as written your iterator will always iterate that high. That's not good for performance. What if you add an max: Option<RawFd> argument tofds? If supplied, the iterator won't check any higher than that. Such an argument could also be useful to highest.
There was a problem hiding this comment.
Added to fds(). I didn't add to highest() in the interest of not introducing unnecessary breakage. If someone wants to get that performance gain, they can use set.fds(Some(my_limit)).next_back(), which isn't terribly hard to write and is pretty clear.
| pub fn contains(&mut self, fd: RawFd) -> bool { | ||
| unsafe { libc::FD_ISSET(fd, &mut self.0) } | ||
| pub fn contains(&self, fd: RawFd) -> bool { | ||
| let mut copy = self.0; |
There was a problem hiding this comment.
I think that for now we should proceed without waiting for rust-lang/libc#1725 . But copying the entire structure on each loop of the iterator is woefully inefficient. I think you should retain the &mut reference instead.
asomers
left a comment
asomers
left a comment
There was a problem hiding this comment.
This is looking better. Please add some tests, too.
|
The CI failure was an intermittent network hiccup. |
|
Build succeeded: |
2 participants
To be more consistent with most Rust APIs and enable cloning of the iterator, I made
FdSet::containsoperate on an immutable borrow instead of a mutable one by copying the set. If this is not desirable, I can roll that back from this PR and focus purely on thefds()method.