-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
DynamicBitSetUnmanaged: .setAll() incorrectly sets all bits, or .iterator incorrectly traverses bits outside the bit_length. #19933
Comments
Would you agree that the solution is to make I just wanted to see if others agree before making any changes. |
After looking at the DynamicBitSetUnmanaged code, I think that there are 2 problems. The first is that setAll() shouldn't change the padding bits. And second is that iterator() shouldn't include padding bits. |
closes ziglang#19933 This patch makes setAll() preserve padding bits and BitSetIterator() ignore padding bits when kind == .set. DynamicBitSetUnmanaged depends on its padding bits being zeroed as stated in the 'masks' field's doc comments. but setAll() was setting all masks, including the padding, to ones. and iterator() wasn't ignoring padding bits when kind == .set, only when .unset.
closes ziglang#19933 This patch makes setAll() preserve padding bits and BitSetIterator() ignore padding bits when kind == .set. DynamicBitSetUnmanaged depends on its padding bits being zeroed as stated in the 'masks' field's doc comments. but setAll() was setting all masks, including the padding, to ones. and iterator() wasn't ignoring padding bits when kind == .set, only when .unset.
closes ziglang#19933 This patch makes setAll() preserve padding bits and BitSetIterator() ignore padding bits when kind == .set. DynamicBitSetUnmanaged depends on its padding bits being zeroed as stated in the 'masks' field's doc comments. but setAll() was setting all masks, including the padding, to ones. and iterator() wasn't ignoring padding bits when kind == .set, only when .unset.
Either change would fix my issue, I was leaning more towards setAll not changing the padding bits because initFull already had that behaviour. And I was unsure if changing the iterator to verify the length might impact performance. I'm new to zig though, so I trust everyone elses judgement on this matter ^^ |
I think its definitely correct to change setAll() since it says in the 'masks' field doc comments
In my PR yesterday I changed iterator() to skip the padding bits. But now, I think that shouldn't be necessary since the padding bits should never be set. And if the user sets them, thats on them. This would make count() and iterator() consistent again whether or not any padding bits are set. So I'm going to revert the iterator() changes from my PR. |
closes ziglang#19933 This patch makes setAll() preserve padding bits. DynamicBitSetUnmanaged depends on its padding bits being zeroed as stated in the 'masks' field's doc comments. but setAll() was setting all masks, including the padding, to ones.
Zig Version
0.13.0-dev.75+5c9eb4081
Steps to Reproduce and Observed Behavior
Code
Running the following program:
Has the following output:
Which means that setAll is setting the bits of the set differently than how initFull sets them. Alternatively it could be an issue with the iterator going outside the range of bit_length.
Other testing done
Expected Behavior
The following output:
The text was updated successfully, but these errors were encountered: