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

impl. AppendTo and AsSlice #195

Merged
merged 1 commit into from
Dec 16, 2024
Merged

Conversation

gaissmai
Copy link
Contributor

added AsSlice() and AppendTo() as convenience functions, if you like it, merge it, otherwise drop the PR

thanks for bitset

@gaissmai
Copy link
Contributor Author

huch, wait a moment, a panic test isn't working as expected

@gaissmai
Copy link
Contributor Author

ready for merge, or drop it if you don't like

// AppendTo appends all set bits to buf and returns the (maybe extended) buf.
//
// See also [BitSet.AsSlice] and [BitSet.NextSetMany].
func (b *BitSet) AppendTo(buf []uint) []uint {
Copy link
Member

Choose a reason for hiding this comment

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

Theoretically, we can overflow uint. It would be nice to mention that what happens if someone has a gigantic bitset.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do

@lemire
Copy link
Member

lemire commented Dec 16, 2024

@gaissmai Please see my comment.

@gaissmai
Copy link
Contributor Author

I'll will push an update in a few minutes

@gaissmai
Copy link
Contributor Author

gaissmai commented Dec 16, 2024

I'm not a native speaker, maybe you will polish my new docstring.

edited: not a native english speaker ;)

@lemire
Copy link
Member

lemire commented Dec 16, 2024

@gaissmai Let me explain what I meant.

On a 32-bit system, uint maxes out at 2**32 - 1.

Meanwhile, the bitset can represent values between 0 and (2**32 - 1) * 64.

Right?

So idx<<log2WordSize might overflow.

It won't overflow in practice. But it is worth saying why. Or, at least, just say 'won't overflow in practice' as a comment.

@gaissmai
Copy link
Contributor Author

hm, yes, but this is not different to NextSet and NetxSetMany

@lemire
Copy link
Member

lemire commented Dec 16, 2024

hm, yes, but this is not different to NextSet and NetxSetMany

Correct. Let me merge this and I will add comments.

@lemire lemire merged commit d231880 into bits-and-blooms:master Dec 16, 2024
15 checks passed
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