Conversation
Contributor
|
Daniel, I can also confirm that this is reproducible with v2.25.0. I haven't merged but if you approve please merge ASAP. Guy |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
NewBSIRetainSetskips the sign plane becauseBitCount()excludes it. SinceClonedelegates to this method, both operations turn negative BSI values into positive values. Include the sign plane in the existing copy-and-intersect loop.Type of Change
Testing
Added public API regression coverage for signed int64 and wider values, zero/positive controls, retained membership and copy independence. The negative-value cases fail before this change.
Passed on macOS arm64:
go test ./...andgo test -tags appengineroaring64andBitSliceIndexingtests with-race, focused copy tests with-race,make unconvertgofmtandgit diff --checkThe template's corpus generation command passes. Its native
-fuzz=FuzzSmatcommand exits immediately: the current tree has no native fuzz target (go test -list '^Fuzz'lists none), so this does not count as a fuzz run. A separate temporary native fuzz test exercised valid public BSI copies against their input values, signs, membership and copy independence for 10 seconds (246,347 executions), with no failure.The root package race run fails in unchanged
TestFrozenFormatwith a checkptr error atserialization_littleendian.go:149. The sameTestFrozenFormatcheckptr failure also reproduces on the pristine base (in its frozen-view conversion at line 137); the full race suite is not claimed as passing.Performance Impact
BenchmarkBSIClonemeasures a BSI with 1,000 mixed positive/negative values. Go 1.26.6 on macOS arm64, median of three 200ms runs:Copying the previously omitted sign plane adds its bitmap storage and one worker (+1,392 B/op, 13 allocations).