-
Notifications
You must be signed in to change notification settings - Fork 33
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
AtomicBitSet Debug crashes #62
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! Thank you for fixing this crash.
I left some suggestions on the AtomicBlock
debug implementation.
.filter_map(|(idx, v)| match v.fetch_or(0, Ordering::Relaxed) { | ||
0 => None, | ||
x => Some((idx, x)), | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fetch_or
will modify the value, I think we just want load
here?
/// clearing of bits. | ||
/// | ||
/// [`BitSet`]: ../struct.BitSet.html | ||
#[derive(Debug)] | ||
// #[derive(Debug)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commented debug derive can be removed
let mut s = f.debug_struct("AtomicBlock"); | ||
s.field("mask", &self.mask); | ||
if let Some(atom) = self.atom.get() { | ||
s.field( | ||
"atom", | ||
&atom | ||
.iter() | ||
.enumerate() | ||
.filter_map(|(idx, v)| match v.fetch_or(0, Ordering::Relaxed) { | ||
0 => None, | ||
x => Some((idx, x)), | ||
}) | ||
.collect::<Vec<(usize, usize)>>(), | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A helper type could be used here to avoid the allocation when collecting into a vec:
let mut s = f.debug_struct("AtomicBlock"); | |
s.field("mask", &self.mask); | |
if let Some(atom) = self.atom.get() { | |
s.field( | |
"atom", | |
&atom | |
.iter() | |
.enumerate() | |
.filter_map(|(idx, v)| match v.fetch_or(0, Ordering::Relaxed) { | |
0 => None, | |
x => Some((idx, x)), | |
}) | |
.collect::<Vec<(usize, usize)>>(), | |
); | |
} | |
struct DebugAtom<'a>(&'a [AtomicUsize; 1 << BITS]); | |
impl Debug for DebugAtom<'_> { | |
fn fmt(&self, f: &mut Formatter) -> Result<(), FormatError> { | |
f.debug_list() | |
.entries(self.0.iter().enumerate().filter_map(|(idx, v)| { | |
match v.load(Ordering::Relaxed) { | |
0 => None, | |
x => Some((idx, x)), | |
} | |
})) | |
.finish() | |
} | |
} | |
let mut s = f.debug_struct("AtomicBlock"); | |
s.field("mask", &self.mask); | |
if let Some(atom) = self.atom.get() { | |
s.field("atom", &DebugAtom(atom)); | |
} |
if let Some(atom) = self.atom.get() { | ||
s.field( | ||
"atom", | ||
&atom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this returns None
I think we would still want the formatting to show that an atom
field exists
e.g. in the None
case perhaps it could be:
s.field("atom", &"None");
or following from my other comment to use a helper type we could do:
s.field("atom", &self.atom.get().map(DebugAtom));
This crashes...
The proposed fix implements Debug for AtomicBlock so that the error doesn't happen - please check the output to make sure it is what you want it to be for internal debugging.
It also implements AtomicBitSet Debug manually to be the output of
iter()
- which I felt was more helpful to users of the library.