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

Update fmt::Debug to produce new info #86

Merged
merged 27 commits into from
Oct 22, 2023
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 20 additions & 6 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,14 +116,14 @@ struct Inner<T> {
/// The number of notified entries, or `usize::MAX` if all of them have been notified.
///
/// If there are no entries, this value is set to `usize::MAX`.
notified: AtomicUsize,
pub notified: AtomicUsize,

/// Inner queue of event listeners.
///
/// On `std` platforms, this is an intrusive linked list. On `no_std` platforms, this is a
/// more traditional `Vec` of listeners, with an atomic queue used as a backup for high
/// contention.
list: sys::List<T>,
pub list: sys::List<T>,
Copy link
Member

Choose a reason for hiding this comment

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

These fields don't need to be pub, I think.

}

impl<T> Inner<T> {
Expand Down Expand Up @@ -171,7 +171,15 @@ impl<T> core::panic::RefUnwindSafe for Event<T> {}

impl<T> fmt::Debug for Event<T> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.write_str("Event { .. }")
let inner = self.inner();
Copy link
Member

Choose a reason for hiding this comment

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

Instead of calling inner(), call try_inner(), and if it returns None output Event(<uninitialized>) using debug_tuple. See here for an example.

let notified_count = unsafe { (*inner).notified.load(Ordering::Relaxed) };
let total_count = unsafe { (*inner).list.total_listeners() };

write!(
f,
"Event {{\n Number of notified listeners: {:?}\n Total number of listeners: {:?}\n}}",
notified_count, total_count
)
Copy link
Member

Choose a reason for hiding this comment

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

To simplify, use debug_struct to construct the output instead of manually formatting like this.

}
}

Expand Down Expand Up @@ -202,7 +210,6 @@ impl<T> Event<T> {
inner: AtomicPtr::new(ptr::null_mut()),
}
}

Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Member

Choose a reason for hiding this comment

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

Can you add back these lines to reduce diff noise?

/// Tell whether any listeners are currently notified.
///
/// # Examples
Expand Down Expand Up @@ -456,7 +463,6 @@ impl Event<()> {
inner: AtomicPtr::new(ptr::null_mut()),
}
}

Copy link
Member

Choose a reason for hiding this comment

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

?

/// Notifies a number of active listeners without emitting a `SeqCst` fence.
///
/// The number is allowed to be zero or exceed the current number of listeners.
Expand Down Expand Up @@ -650,7 +656,15 @@ unsafe impl<T: Send> Sync for EventListener<T> {}

impl<T> fmt::Debug for EventListener<T> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.write_str("EventListener { .. }")
let mut ds = f.debug_struct("EventListener");

if self.is_listening() {
ds.field("Listening", &"Yes");
} else {
ds.field("Listening", &"No");
}
Copy link
Member

Choose a reason for hiding this comment

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

Instead of yes and no, I would just use true and false, which can be done by just using self.is_listening() as the field here.


ds.finish()
}
}

Expand Down
8 changes: 8 additions & 0 deletions src/no_std.rs
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,14 @@ impl<T> List<T> {
queue: concurrent_queue::ConcurrentQueue::unbounded(),
}
}
pub fn total_listeners(&self) -> usize {
self.inner
.try_lock()
.as_ref()
.map(|lock| &**lock)
.map(|listener_slab| listener_slab.listeners.len())
Copy link
Member

Choose a reason for hiding this comment

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

Don't use map() twice here, just get listeners from lock itself.

.unwrap_or(0)
}
Copy link
Member

Choose a reason for hiding this comment

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

This returns something different from the total_listeners in std.rs.

}

/// The guard returned by [`Inner::lock`].
Expand Down
4 changes: 4 additions & 0 deletions src/std.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ impl<T> List<T> {
notified: 0,
}))
}
// Accessor method because fields are private, not sure how to go around it
pub fn total_listeners(&self) -> Result<usize, &str> {
self.0.lock().map(|mutex| mutex.len).map_err(|_| "<locked>")
Copy link
Member

Choose a reason for hiding this comment

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

Can you do try_lock() to avoid contention?

}
}

impl<T> crate::Inner<T> {
Expand Down
Loading