fix Issue 10948 - BitArray.opEquals is invalid #2204
Conversation
|
This is a reboot of #1551 , there is likely relevant comments there. |
|
Sorry, I didn't notice that @nbelov made a pull request before. From what I could gather, the very thorough unit test in that PR triggered a test failure of unclear cause. It didn't seem to be connected to the fix of this bug, so unless the failure appears again this fix should probably go in. I will try that expensive test @nbelov wrote and try to trigger that failure again. If I find something, I will open another bug report. |
|
There's already a bug report for it and I've been trying to narrow down the cause in: #1551 By coincidence, at the moment I'm creating a Freebsd 32bit VM to try and solve this once and for all. |
|
Well, if this doesn't break anything, I'm pulling it. The code is obviously more correct than the old code. |
| @@ -884,7 +884,7 @@ struct BitArray | |||
| } | |||
|
|
|||
| n = this.length & (bitsPerSizeT-1); | |||
| size_t mask = (1 << n) - 1; | |||
| size_t mask = (cast(size_t)1 << n) - 1; | |||
There was a problem hiding this comment.
Nitpick: uniform initialization now allows:
size_t mask = (size_t(1) << n) - 1;I think it reads better.
|
@monarchdodra BTW, I tracked down the source of the failure: https://issues.dlang.org/show_bug.cgi?id=11435#c5 |
|
Sorry, there was more in that commit than I wanted. I'll fix it and squash the commits. |
https://issues.dlang.org/show_bug.cgi?id=10948 Implemented the fix as proposed by nbelov on Bugzilla. Included are unit tests which would have triggered this bug and also fixed it in opCmp. The issue was that the 32 bit expression "(1 << n)" was treated as being of size_t, which lead to incorrect bit patterns on 64 bit environments.
|
I have changed the casts to uniform initialization and squashed the commits into one. This should be good to go now. |
|
Won't hurt to pull I guess. |
fix Issue 10948 - BitArray.opEquals is invalid
https://issues.dlang.org/show_bug.cgi?id=10948
Implemented the fix as proposed by nbelov on Bugzilla. Included are unit tests which would have triggered this bug and also fixed it in opCmp.
The issue was that the 32 bit expression "(1 << n)" was treated as being of size_t, which lead to incorrect bit patterns on 64 bit environments.