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

AtomicBitSet Debug crashes #62

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

funnisimo
Copy link

This crashes...

use hibitset::AtomicBitSet;

fn main() {
    let bitset = AtomicBitSet::new();
    println!("bitset = {:?}", bitset);
}

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.

Copy link
Contributor

@Imberflur Imberflur left a 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.

Comment on lines +346 to +349
.filter_map(|(idx, v)| match v.fetch_or(0, Ordering::Relaxed) {
0 => None,
x => Some((idx, x)),
})
Copy link
Contributor

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)]
Copy link
Contributor

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

Comment on lines +338 to +352
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)>>(),
);
}
Copy link
Contributor

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:

Suggested change
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));
}

Comment on lines +340 to +343
if let Some(atom) = self.atom.get() {
s.field(
"atom",
&atom
Copy link
Contributor

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));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants