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

Add is_clear method #79

Merged
merged 1 commit into from
Jul 3, 2022
Merged

Add is_clear method #79

merged 1 commit into from
Jul 3, 2022

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?

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@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.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
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