Fix bug in BlistList for two ranges that could lead to wrong results
#3689
Conversation
This file contains 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
Several results were wrong, and had been wrong since at least GAP 4.4.
E.g. before this commit:
gap> BlistList([0..3], [-3..-1]);
[ true, true, true, false ]
Now we get the correct result
gap> BlistList([0,1,2,3], [-3..-1]);
[ false, false, false, false ]
One major issue was the use of unsigned variables to store signed data. Two
variables were only of type `long` instead of `Int`, but on 64bit, this could
lead to further issues.
The logic handling the intersection was broken and overly complicated. The
rewritten logic should be simpler.
Finally, there is a tiny optimization enabled by switching from 1-based
indexing to 0-based.
fingolfin
added
kind: bug
fingolfin
commented
Oct 6, 2019
| // compute bounds | ||
| i = t - s; | ||
| j = lenSub + i; | ||
| if (i < 0) |
There was a problem hiding this comment.
Note that it is crucial to compute i and j before capping the bounds; clearly the previous two lines do not commute; yet in the old code, they were in the wrong order.
fingolfin
commented
Oct 6, 2019
| long s, t; /* elements of a range */ | ||
| Int lenSub; /* logical length of sublist */ | ||
| Int i, j, k, l; /* loop variables */ | ||
| Int s, t; /* elements of a range */ |
There was a problem hiding this comment.
s, t were just of type long, so in 64 bit mode, with compilers where long is only 32 bit (basically anywhere except some Microsoft / Windows compilers), there could be overflow here. I didn't add tests for that, though.
fingolfin
commented
Oct 6, 2019
| for ( ; k+BIPEB < j; k += BIPEB ) | ||
| ptrBlist[k/BIPEB] = ~(UInt)0; | ||
| for ( ; k < j; k++ ) | ||
| ptrBlist[k/BIPEB] |= (1UL << k%BIPEB); |
There was a problem hiding this comment.
Note that k-1 changed to k by adjusting the bounds. I hope the optimizer would have produced identical results in both cases; but the result is certainly slightly easier to read.
fingolfin
added
the
release notes: to be added
ChrisJefferson
approved these changes
Oct 6, 2019
|
Backported to stable-4.11 in 8761c8f |
fingolfin
changed the title
BlistList for two ranges that could lead to wrong results
fingolfin
changed the title
BlistList for two ranges that could lead to wrong resultsBlistList for two ranges that could lead to wrong results
fingolfin
added
release notes: added
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Several results were wrong, and had been wrong since at least GAP 4.4.
E.g. before this commit:
Now we get the correct result
One major issue was the use of unsigned variables to store signed data. Two
variables were only of type
longinstead ofInt, but on 64bit, this couldlead to further issues.
The logic handling the intersection was broken and overly complicated. The
rewritten logic should be simpler.
Finally, there is a tiny optimization enabled by switching from 1-based
indexing to 0-based.