RFR JDK-6321472: Add CRC-32C API
Staffan Friberg
staffan.friberg at oracle.com
Fri Nov 21 14:50:16 UTC 2014
Hi David,
Thanks for reviewing.
> CRC32C.java:
> 56 /**
> 57 * CRC-32C Polynom
> 58 */
>
> “Polynomial”, perhaps?
Done
> You did test this on both big- and little-endian boxes, right?
> It looks right to me, but this stuff is brain-hurty on mirror-image dyslexia and fenceposts.
Yes, as part of the push I added a tests for both CRC32 and CRC32C and
they pass on all platforms (and were really helpful when implementing).
> It might not hurt to mention that byteTable is for byte-at-a-time calculations and is always
> the same, whereas the other 8 tables are for bulk operations and will have contents
> dependent on the endianness of the platform.
Did a slight reorder to separate the single table and the bulk tables
and added comments according to your suggestion.
> It would be a good idea to mention that the high-order terms of polynomials
> are stored in the low-order bits of the integers in the tables.
>
> You could get the bit reversal out of the init code if you bit-reversed CRC32C_POLY instead and wrote it
>
> 78 int r = index;
> // High-order polynomial term stored in LSB of r.
> 79 for (int i = 0; i < Byte.SIZE; i++) {
> 80 if ((r & 1) != 0) {
> 81 r = (r >>> 1) ^ REVERSED_CRC32C_POLY;
> 82 } else {
> 83 r >>>= 1;
> 84 }
> 85 }
> 86 byteTables[0][index] = r;
>
> at least, it seems that way to me.
Agree, and so does the tests... :)
Here is the last webrev with the above changes,
http://cr.openjdk.java.net/~sfriberg/JDK-6321472/webrev.09
I will ask Sherman to sponsor and push the change, since Joe mentioned
only a single Reviewer is currently OK in JDK 9.
Thanks,
Staffan
On 11/20/2014 08:41 PM, David Chase wrote:
> On 2014-11-20, at 7:45 AM, Staffan Friberg <staffan.friberg at oracle.com> wrote:
>
>> Hi,
>>
>> Anyone who can be the second Reviewer?
>>
>> Thanks,
>> Staffan
> I can review, but I am not a Reviewer.
>
> CRC32C.java:
> 56 /**
> 57 * CRC-32C Polynom
> 58 */
>
> “Polynomial”, perhaps?
>
> You did test this on both big- and little-endian boxes, right?
> It looks right to me, but this stuff is brain-hurty on mirror-image dyslexia and fenceposts.
>
> It might not hurt to mention that byteTable is for byte-at-a-time calculations and is always
> the same, whereas the other 8 tables are for bulk operations and will have contents
> dependent on the endianness of the platform.
>
> It would be a good idea to mention that the high-order terms of polynomials
> are stored in the low-order bits of the integers in the tables.
>
> You could get the bit reversal out of the init code if you bit-reversed CRC32C_POLY instead and wrote it
>
> 78 int r = index;
> // High-order polynomial term stored in LSB of r.
> 79 for (int i = 0; i < Byte.SIZE; i++) {
> 80 if ((r & 1) != 0) {
> 81 r = (r >>> 1) ^ REVERSED_CRC32C_POLY;
> 82 } else {
> 83 r >>>= 1;
> 84 }
> 85 }
> 86 byteTables[0][index] = r;
>
> at least, it seems that way to me.
>
> David
>
More information about the core-libs-dev
mailing list