Skip to content

Conversation

@nicopap
Copy link
Contributor

@nicopap nicopap commented Jul 1, 2022

When using this library, the intent of checking if all elements in a bitset are disabled is better expressed with a specialized method. I doubt it's particularly more performant than bitset.count_ones(..) == 0, but when reading code using FixedBitSet, it's easier to understand what is going on with a bitset.is_clear() than a bitset.count_ones(..) == 0.

I also added docs so that it's less easy to get confused as to what "empty" means (I think the confusion was raised multiple times in the past)

@jrraymond
Copy link
Collaborator

Thanks. Can you please add some tests for this function?

@nicopap
Copy link
Contributor Author

nicopap commented Jul 3, 2022

Thank for the review! Very important to keep thing names consistent in documentation. I indeed picked up "element" by just looking at the iterator methods. I'll fix that.

A question: the docs use both "enabled/disabled" and "set/unset" for value = 1/0. You seem to prefer "set/unset", and I'll follow this preference, but I prefer "enabled/disabled" since "set" can also mean a set of values.

@nicopap
Copy link
Contributor Author

nicopap commented Jul 3, 2022

I'm very happy with the second pass on the docs. Although, in insight, I'm a bit skittish about both updating the doc on is_empty and len and adding is_clear.

@nicopap nicopap requested a review from jrraymond July 3, 2022 10:47
@jrraymond
Copy link
Collaborator

"enabled/disabled" is ok. Using "bit" instead of element is more important.

Don't worry about updating the doc on is_empty and len.

Copy link
Collaborator

@jrraymond jrraymond left a comment

Choose a reason for hiding this comment

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

Thanks! As a final step, could you please add some test for is_clear()?

@jrraymond jrraymond merged commit f9c9c09 into petgraph:master Jul 3, 2022
@nicopap
Copy link
Contributor Author

nicopap commented Jul 4, 2022

Thank you for the directions and the merge! Was the doc example code enough test?

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.

2 participants