Small Bugfix: Remove possibility of bitshifting negative integers #1111
Conversation
lroberts36
changed the title
lroberts36
changed the title
| auto low = (l(dir) << level_shift_this) - 1; | ||
| auto low = l(dir) * block_size_this - 1; | ||
| auto hi = low + block_size_this + 1; | ||
|
|
||
| auto low_in = (in.l(dir) << level_shift_in); | ||
| auto low_in = in.l(dir) * block_size_in; | ||
| auto hi_in = low_in + block_size_in - 1; |
There was a problem hiding this comment.
Out of curiosity, where does the asymmetry come from (i.e., that low/hi is offset by-1/+1 and low_in, hi_in by 0,-1)?
Would be great to also add a short comment in the code.
There was a problem hiding this comment.
It just has to do with how the overlap is checked, i.e. the bounds of this are increased by one in the positive and negative direction to create a one layer thick ghost region.
|
btw you got the Schnapszahl ;) |
|
@pgrete: I am not sure why this worked previously, but I did not look carefully at what happened with the negative but shift. I think this should have been hit reasonably often. Haha, I had to look up schnapszahl... |
|
The test now reliably failed twice. Does anyone see how this changeset could result in that error? I don't. |
It's a NumPy 2.0 API breakage 😞... |
|
The test scripts need to be updated: https://numpy.org/devdocs/numpy_2_0_migration_guide.html#numpy-2-migration-guide |
|
@pgrete and @BenWibking: Apparently this worked because (at least clang and gcc) bitshift negative numbers in the expected way, i.e |
|
CI fix here: #1116 |
great, thanks for confirming! |
PR Summary
This PR fixes the bitshift related undefined behavior described in #1107 by multiplying by a power of two explicitly rather than performing the same operation (on positive numbers) via a bitshift.
PR Checklist