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

Implement custom behavior for PartialEq, Eq, PartialOrd, Ord and Hash #47

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

Conversation

sitegui
Copy link

@sitegui sitegui commented May 30, 2020

Hi,

Following up on #44, here is my take on implementing PartialEq, Eq, PartialOrd, Ord and Hash. This PR goes beyond the scope of the issue, so all those related traits would be in sync.

This is a behavior change that, in practice, makes FixedBitSet behave like a BTreeSet<usize>, whose elements are the items returned by FixedBitSet::ones(). The change can be breaking for some pieces of code that depended on the old behavior, even if it was undocumented and arguably surprising.

For example, using FixedBitSet in map will behave differently:

let mut map = HashMap::new();
map.insert(FixedBitSet::with_capacity(0), 17);
map.insert(FixedBitSet::with_capacity(10), 17);
dbg!(map.len()); // before = 2, now = 1

Fix #44

Please tell me what you think,
Best regards,

The observed behavior for those traits is the same as the behavior of the
standard library's `BTreeSet<usize>`
src/lib.rs Outdated
{
#[inline]
fn eq(&self, other: &Self) -> bool {
self.ones().eq(other.ones())
Copy link
Member

Choose a reason for hiding this comment

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

While conceptually this algorithm for comparison might be good, I'm afraid that it is slow. Comparison should be based on comparing the blocks, I would guess.

Copy link
Author

Choose a reason for hiding this comment

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

It makes sense!

I've rewrote the code to extract the trimmed slice of blocks instead of using the iterator of ones.

@jrraymond
Copy link
Collaborator

This results in a somewhat awkward condition that FixedBitSets with different sizes would compare equal:

    /// Return the length of the `FixedBitSet` in bits.
    #[inline]
    pub fn len(&self) -> usize { self.length }

We could also update the len function to be the number of set bits, instead of just the number of bits.

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.

PartialEq implementation depends on capacity
3 participants