Update fmt::Debug to produce new info #86
Conversation
There was a problem hiding this comment.
I definitely like Event including the number of notified listeners and the number of total listeners. However, the debug output fields should use snake_case, e.g. notified_listeners: 7.
For EventListener, State indicates the states that the listener can be in. I think that this state should be reflected in the debug output.
src/lib.rs
Outdated
| 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>, |
There was a problem hiding this comment.
These fields don't need to be pub, I think.
src/lib.rs
Outdated
| @@ -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(); | |||
There was a problem hiding this comment.
Instead of calling inner(), call try_inner(), and if it returns None output Event(<uninitialized>) using debug_tuple. See here for an example.
src/lib.rs
Outdated
| if self.is_listening() { | ||
| ds.field("Listening", &"Yes"); | ||
| } else { | ||
| ds.field("Listening", &"No"); | ||
| } |
There was a problem hiding this comment.
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.
src/lib.rs
Outdated
| @@ -202,7 +210,6 @@ impl<T> Event<T> { | |||
| inner: AtomicPtr::new(ptr::null_mut()), | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Can you add back these lines to reduce diff noise?
src/lib.rs
Outdated
| @@ -456,7 +463,6 @@ impl Event<()> { | |||
| inner: AtomicPtr::new(ptr::null_mut()), | |||
| } | |||
| } | |||
|
|
|||
src/no_std.rs
Outdated
| pub fn total_listeners(&self) -> usize { | ||
| self.inner | ||
| .try_lock() | ||
| .as_ref() | ||
| .map(|lock| &**lock) | ||
| .map(|listener_slab| listener_slab.listeners.len()) | ||
| .unwrap_or(0) | ||
| } |
There was a problem hiding this comment.
This returns something different from the total_listeners in std.rs.
src/no_std.rs
Outdated
| .map(|lock| &**lock) | ||
| .map(|listener_slab| listener_slab.listeners.len()) |
There was a problem hiding this comment.
Don't use map() twice here, just get listeners from lock itself.
src/lib.rs
Outdated
| write!( | ||
| f, | ||
| "Event {{\n Number of notified listeners: {:?}\n Total number of listeners: {:?}\n}}", | ||
| notified_count, total_count | ||
| ) |
There was a problem hiding this comment.
To simplify, use debug_struct to construct the output instead of manually formatting like this.
|
Rebase on the current |
|
Let me know if I missed something |
src/lib.rs
Outdated
| @@ -202,7 +210,6 @@ impl<T> Event<T> { | |||
| inner: AtomicPtr::new(ptr::null_mut()), | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Can you add back these lines to reduce diff noise?
src/std.rs
Outdated
| @@ -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>") | |||
There was a problem hiding this comment.
Can you do try_lock() to avoid contention?
Co-authored-by: John Nunley <[email protected]>
src/lib.rs
Outdated
| .finish() | ||
| } | ||
| None => f | ||
| .debug_tuple("event") |
Co-authored-by: John Nunley <[email protected]>
|
|
cc #86, it's harder to get this info on no_std so I've ignored it for now Signed-off-by: John Nunley <[email protected]>
cc #86, it's harder to get this info on no_std so I've ignored it for now Signed-off-by: John Nunley <[email protected]>
cc #86, it's harder to get this info on no_std so I've ignored it for now Signed-off-by: John Nunley <[email protected]>
Closes #79
Let me know what to change