-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Fix BitArray.CopyTo to byte[] regression - update offset for extra bits. #40441
Conversation
@@ -657,7 +657,7 @@ public void CopyTo(Array array, int index) | |||
Debug.Assert(span.Length > 0); | |||
Debug.Assert(m_array.Length > quotient); | |||
// mask the final byte | |||
span[span.Length - 1] = (byte)((m_array[quotient] >> (remainder * 8)) & ((1 << (int)extraBits) - 1)); | |||
span[remainder] = (byte)((m_array[quotient] >> (remainder * 8)) & ((1 << (int)extraBits) - 1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before the change from #33367, this used to be as follows:
corefx/src/System.Collections/src/System/Collections/BitArray.cs
Lines 545 to 554 in 8716f1a
// copy all the perfectly-aligned bytes | |
for (int i = 0; i < arrayLength; i++) | |
b[index + i] = (byte)((m_array[i / 4] >> ((i % 4) * 8)) & 0x000000FF); // Shift to bring the required byte to LSB, then mask | |
if (extraBits > 0) | |
{ | |
// mask the final byte | |
int i = arrayLength; | |
b[index + i] = (byte)((m_array[i / 4] >> ((i % 4) * 8)) & ((1 << extraBits) - 1)); | |
} |
Before, we were writing one byte at a time, and then if the bit array contained extra bits to be copied over, we used the next index to write those bits. Now, we are writing 4 bytes at a time, and if we have 1-3 bytes left to write, we have the switch on remainder below. Beyond that, any extra bits left to copy (i.e. < 1 byte) should go at offset remainder
.
I believe using remainder
as the offset to write the last bits to is correct (and the test pass). Please review/verify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way
(byte)((m_array[quotient] >> (remainder * 8)) & ((1 << (int)extraBits) - 1));
can be optimized in JIT: https://godbolt.org/z/XQ9Mpe (or at least with Bmi2.ZeroHighBits
)
current codegen: sharplab.io
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @AndyAyersMS
/azp run corefx-ci |
Azure Pipelines successfully started running 1 pipeline(s). |
} | ||
} | ||
|
||
// https://github.com/dotnet/corefx/issues/39929 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these comments necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the test name speaks for itself 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @ahsonkhan
…ts. (dotnet/corefx#40441) Commit migrated from dotnet/corefx@2547b52
Fixes dotnet/core#3194 and https://github.com/dotnet/corefx/issues/39929
We need to port this to release/3.0
cc @stephentoub, @jkotas, @safern, @MichaelL79, @carlossanlop, @Clockwork-Muse, @ericstj, @danielValdezR