Review comments

- Make set_* take a boolean for the value of the flag - Make Events !Sync - Fix visibility modifiers - Inline more methods - Use a better strategy for testing Signed-off-by: John Nunley <[email protected]>
smol-rs · Aug 12, 2023 · 1a10d2a · 1a10d2a
1 parent a568261
commit 1a10d2a
Show file tree

Hide file tree

Showing 14 changed files with 166 additions and 71 deletions.
diff --git a/src/epoll.rs b/src/epoll.rs
@@ -334,28 +334,33 @@ pub struct EventExtra {
 
 impl EventExtra {
     /// Create an empty version of the data.
+    #[inline]
     pub fn empty() -> EventExtra {
         EventExtra {
             flags: epoll::EventFlags::empty(),
         }
     }
 
     /// Add the interrupt flag to this event.
-    pub fn set_hup(&mut self) {
-        self.flags |= epoll::EventFlags::HUP;
+    #[inline]
+    pub fn set_hup(&mut self, active: bool) {
+        self.flags.set(epoll::EventFlags::HUP, active);
     }
 
     /// Add the priority flag to this event.
-    pub fn set_pri(&mut self) {
-        self.flags |= epoll::EventFlags::PRI;
+    #[inline]
+    pub fn set_pri(&mut self, active: bool) {
+        self.flags.set(epoll::EventFlags::PRI, active);
     }
 
     /// Tell if the interrupt flag is set.
+    #[inline]
     pub fn is_hup(&self) -> bool {
         self.flags.contains(epoll::EventFlags::HUP)
     }
 
     /// Tell if the priority flag is set.
+    #[inline]
     pub fn is_pri(&self) -> bool {
         self.flags.contains(epoll::EventFlags::PRI)
     }

diff --git a/src/iocp/afd.rs b/src/iocp/afd.rs
@@ -89,6 +89,15 @@ impl AfdPollMask {
     pub(crate) fn intersects(self, other: AfdPollMask) -> bool {
         (self.0 & other.0) != 0
     }
+
+    /// Sets a flag.
+    pub(crate) fn set(&mut self, other: AfdPollMask, value: bool) {
+        if value {
+            *self |= other;
+        } else {
+            self.0 &= !other.0;
+        }
+    }
 }
 
 impl fmt::Debug for AfdPollMask {

diff --git a/src/iocp/mod.rs b/src/iocp/mod.rs
@@ -622,24 +622,24 @@ unsafe impl Send for Events {}
 
 impl Events {
     /// Creates an empty list of events.
-    pub(super) fn with_capacity(cap: usize) -> Events {
+    pub fn with_capacity(cap: usize) -> Events {
         Events {
             packets: Vec::with_capacity(cap),
         }
     }
 
     /// Iterate over I/O events.
-    pub(super) fn iter(&self) -> impl Iterator<Item = Event> + '_ {
+    pub fn iter(&self) -> impl Iterator<Item = Event> + '_ {
         self.packets.iter().copied()
     }
 
     /// Clear the list of events.
-    pub(super) fn clear(&mut self) {
+    pub fn clear(&mut self) {
         self.packets.clear();
     }
 
     /// The capacity of the list of events.
-    pub(super) fn capacity(&self) -> usize {
+    pub fn capacity(&self) -> usize {
         self.packets.capacity()
     }
 }
@@ -653,30 +653,35 @@ pub struct EventExtra {
 
 impl EventExtra {
     /// Create a new, empty version of this struct.
+    #[inline]
     pub fn empty() -> EventExtra {
         EventExtra {
             flags: AfdPollMask::empty(),
         }
     }
 
     /// Is this a HUP event?
+    #[inline]
     pub fn is_hup(&self) -> bool {
         self.flags.intersects(AfdPollMask::ABORT)
     }
 
     /// Is this a PRI event?
+    #[inline]
     pub fn is_pri(&self) -> bool {
         self.flags.intersects(AfdPollMask::RECEIVE_EXPEDITED)
     }
 
     /// Set up a listener for HUP events.
-    pub fn set_hup(&mut self) {
-        self.flags |= AfdPollMask::ABORT;
+    #[inline]
+    pub fn set_hup(&mut self, active: bool) {
+        self.flags.set(AfdPollMask::ABORT, active);
     }
 
     /// Set up a listener for PRI events.
-    pub fn set_pri(&mut self) {
-        self.flags |= AfdPollMask::RECEIVE_EXPEDITED;
+    #[inline]
+    pub fn set_pri(&mut self, active: bool) {
+        self.flags.set(AfdPollMask::RECEIVE_EXPEDITED, active);
     }
 }
 

diff --git a/src/kqueue.rs b/src/kqueue.rs
@@ -268,26 +268,31 @@ pub struct EventExtra;
 
 impl EventExtra {
     /// Create a new, empty version of this struct.
+    #[inline]
     pub fn empty() -> EventExtra {
         EventExtra
     }
 
     /// Set the interrupt flag.
-    pub fn set_hup(&mut self) {
+    #[inline]
+    pub fn set_hup(&mut self, _value: bool) {
         // No-op.
     }
 
     /// Set the priority flag.
-    pub fn set_pri(&mut self) {
+    #[inline]
+    pub fn set_pri(&mut self, _value: bool) {
         // No-op.
     }
 
     /// Is the interrupt flag set?
+    #[inline]
     pub fn is_hup(&self) -> bool {
         false
     }
 
     /// Is the priority flag set?
+    #[inline]
     pub fn is_pri(&self) -> bool {
         false
     }

diff --git a/src/lib.rs b/src/lib.rs
@@ -65,8 +65,10 @@
     html_logo_url = "https://raw.githubusercontent.com/smol-rs/smol/master/assets/images/logo_fullsize_transparent.png"
 )]
 
+use std::cell::Cell;
 use std::fmt;
 use std::io;
+use std::marker::PhantomData;
 use std::num::NonZeroUsize;
 use std::sync::atomic::{AtomicBool, Ordering};
 use std::sync::Mutex;
@@ -242,8 +244,9 @@ impl Event {
     /// - Event Ports
     ///
     /// On other platforms, this function is a no-op.
-    pub fn set_interrupt(&mut self) {
-        self.extra.set_hup();
+    #[inline]
+    pub fn set_interrupt(&mut self, active: bool) {
+        self.extra.set_hup(active);
     }
 
     /// Add interruption events to this interest.
@@ -259,8 +262,9 @@ impl Event {
     /// - Event Ports
     ///
     /// On other platforms, this function is a no-op.
+    #[inline]
     pub fn with_interrupt(mut self) -> Self {
-        self.set_interrupt();
+        self.set_interrupt(true);
         self
     }
 
@@ -277,8 +281,9 @@ impl Event {
     /// - Event Ports
     ///
     /// On other platforms, this function is a no-op.
-    pub fn set_priority(&mut self) {
-        self.extra.set_pri();
+    #[inline]
+    pub fn set_priority(&mut self, active: bool) {
+        self.extra.set_pri(active);
     }
 
     /// Add priority events to this interest.
@@ -294,8 +299,9 @@ impl Event {
     /// - Event Ports
     ///
     /// On other platforms, this function is a no-op.
+    #[inline]
     pub fn with_priority(mut self) -> Self {
-        self.set_priority();
+        self.set_priority(true);
         self
     }
 
@@ -312,6 +318,7 @@ impl Event {
     /// - Event Ports
     ///
     /// On other platforms, this always returns `false`.
+    #[inline]
     pub fn is_interrupt(&self) -> bool {
         self.extra.is_hup()
     }
@@ -329,9 +336,25 @@ impl Event {
     /// - Event Ports
     ///
     /// On other platforms, this always returns `false`.
+    #[inline]
     pub fn is_priority(&self) -> bool {
         self.extra.is_pri()
     }
+
+    /// Remove any extra information from this event.
+    #[inline]
+    pub fn clear_extra(&mut self) {
+        self.extra = sys::EventExtra::empty();
+    }
+
+    /// Get a version of this event with no extra information.
+    ///
+    /// This is useful for comparing events with `==`.
+    #[inline]
+    pub fn with_no_extra(mut self) -> Self {
+        self.clear_extra();
+        self
+    }
 }
 
 /// Waits for I/O events.
@@ -680,6 +703,10 @@ impl Poller {
 /// A container for I/O events.
 pub struct Events {
     events: sys::Events,
+
+    /// This is intended to be used from &mut, thread locally, so we should make it !Sync
+    /// for consistency with the rest of the API.
+    _not_sync: PhantomData<Cell<()>>,
 }
 
 impl Default for Events {
@@ -728,6 +755,7 @@ impl Events {
     pub fn with_capacity(capacity: NonZeroUsize) -> Self {
         Self {
             events: sys::Events::with_capacity(capacity.get()),
+            _not_sync: PhantomData,
         }
     }
 
@@ -975,3 +1003,17 @@ cfg_if! {
 fn unsupported_error(err: impl Into<String>) -> io::Error {
     io::Error::new(io::ErrorKind::Unsupported, err.into())
 }
+
+fn _assert_send_and_sync() {
+    fn assert_send<T: Send>() {}
+    fn assert_sync<T: Sync>() {}
+
+    assert_send::<Poller>();
+    assert_sync::<Poller>();
+
+    assert_send::<Event>();
+    assert_sync::<Event>();
+
+    assert_send::<Events>();
+    // Events can be !Sync
+}
diff --git a/src/poll.rs b/src/poll.rs
@@ -397,28 +397,33 @@ pub struct EventExtra {
 
 impl EventExtra {
     /// Creates an empty set of extra information.
+    #[inline]
     pub fn empty() -> Self {
         Self {
             flags: PollFlags::empty(),
         }
     }
 
     /// Set the interrupt flag.
-    pub fn set_hup(&mut self) {
-        self.flags.set(PollFlags::HUP, true);
+    #[inline]
+    pub fn set_hup(&mut self, value: bool) {
+        self.flags.set(PollFlags::HUP, value);
     }
 
     /// Set the priority flag.
-    pub fn set_pri(&mut self) {
-        self.flags.set(PollFlags::PRI, true);
+    #[inline]
+    pub fn set_pri(&mut self, value: bool) {
+        self.flags.set(PollFlags::PRI, value);
     }
 
     /// Is this an interrupt event?
+    #[inline]
     pub fn is_hup(&self) -> bool {
         self.flags.contains(PollFlags::HUP)
     }
 
     /// Is this a priority event?
+    #[inline]
     pub fn is_pri(&self) -> bool {
         self.flags.contains(PollFlags::PRI)
     }

diff --git a/src/port.rs b/src/port.rs
@@ -220,28 +220,33 @@ pub struct EventExtra {
 
 impl EventExtra {
     /// Create a new, empty version of this struct.
+    #[inline]
     pub fn empty() -> EventExtra {
         EventExtra {
             flags: PollFlags::empty(),
         }
     }
 
     /// Set the interrupt flag.
-    pub fn set_hup(&mut self) {
-        self.flags.set(PollFlags::HUP, true);
+    #[inline]
+    pub fn set_hup(&mut self, value: bool) {
+        self.flags.set(PollFlags::HUP, value);
     }
 
     /// Set the priority flag.
-    pub fn set_pri(&mut self) {
-        self.flags.set(PollFlags::PRI, true);
+    #[inline]
+    pub fn set_pri(&mut self, value: bool) {
+        self.flags.set(PollFlags::PRI, value);
     }
 
     /// Is this an interrupt event?
+    #[inline]
     pub fn is_hup(&self) -> bool {
         self.flags.contains(PollFlags::HUP)
     }
 
     /// Is this a priority event?
+    #[inline]
     pub fn is_pri(&self) -> bool {
         self.flags.contains(PollFlags::PRI)
     }

diff --git a/tests/concurrent_modification.rs b/tests/concurrent_modification.rs
@@ -34,8 +34,10 @@ fn concurrent_add() -> io::Result<()> {
     result?;
 
     assert_eq!(events.len(), 1);
-    assert!(events.iter().next().unwrap().readable);
-    assert!(!events.iter().next().unwrap().writable);
+    assert_eq!(
+        events.iter().next().unwrap().with_no_extra(),
+        Event::readable(0)
+    );
 
     Ok(())
 }
@@ -66,8 +68,10 @@ fn concurrent_modify() -> io::Result<()> {
         .collect::<io::Result<()>>()?;
 
     assert_eq!(events.len(), 1);
-    assert!(events.iter().next().unwrap().readable);
-    assert!(!events.iter().next().unwrap().writable);
+    assert_eq!(
+        events.iter().next().unwrap().with_no_extra(),
+        Event::readable(0)
+    );
 
     Ok(())
 }