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