RFR JDK-6321472: Add CRC-32C API

[Stanimir Simeonoff]

Hi,

I don't quite see the point of this line:

  private transient final static ByteOrder ENDIAN
            = ByteOrder.nativeOrder().equals(ByteOrder.BIG_ENDIAN)
                    ? ByteOrder.BIG_ENDIAN : ByteOrder.LITTLE_ENDIAN;

should be just private static final ByteOrder ENDIAN =
ByteOrder.nativeOrder();

Also you don't need equals for enums alikes, just reference comparison (==)
as they are constants.
Alternatively you can just replace it with a boolean as there are no more
endianess options.

Stanimir

On Fri, Oct 17, 2014 at 9:58 PM, Staffan Friberg <staffan.friberg at oracle.com
> wrote:

> On 10/17/2014 04:05 AM, Alan Bateman wrote:
>
>> On 17/10/2014 02:42, Staffan Friberg wrote:
>>
>>> Hi,
>>>
>>> This RFE adds a CRC-32C class. It implements Checksum so it will have
>>> the same API CRC-32, but use a different polynomial when calculating the
>>> CRC checksum.
>>>
>>> CRC-32C implementation uses slicing-by-8 to achieve high performance
>>> when calculating the CRC value.
>>>
>>> A part from adding the new class, java.util.zip.CRC32C, I have also
>>> added two default methods to Checksum. These are methods that were added to
>>> Adler32 and CRC32 in JDK 8 but before default methods were added, which was
>>> why they were only added to the implementors and not the interface.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-6321472
>>> Webrev: http://cr.openjdk.java.net/~sfriberg/JDK-6321472/webrev.00
>>>
>>>  I looked over the javadoc, I haven't found time to study the
>> implementation in CRC32C closely yet. Hopefully Sherman will be able to
>> review as he I think he has prototyped several CRC32C implementations.
>>
>> On Checksum#update(ByteBuffer) then a suggestion for:
>>  "The checksum is updated using buffer.remaining, starting a
>> buffer.position"
>> is to replace it with:
>>   "The checksum is updated with the remaining bytes in the buffer,
>> starting at the buffer's position."
>>
>
> Yes that reads much better. Updated CRC32 and Adler32 as well since they
> have the same text.
>
>
>> In the @implNote then I wonder if it would be better to just leave out
>> the note about when the invariants are broken, we don't do that in other
>> places where breakage this is detected.  Also I wonder if the note about
>> "For best performance, ..." should be an @apiNote.
>>
> Done, removed the assert comment and changed the performance note to an
> @apiNote.
>
>
>> Should CRC32C be final unless we have a good reason for it not to be
>> final?
>>
> I simply followed what was used for CRC32 and Adler32, but I don't see a
> reason for not making it final. Guess it is too late to make those two
> classes final though?
>
>
>> In CRC32C then I assume you don't need the <p>/<p> for the first
>> statement. The @see Checksum might not be too interesting here given that
>> it will be linked to anyway by way of implement Checksum.
>>
>
> Removed @see in all three classes.
>
>
>> I think it's more usual to list the @param tags before the @throws.
>>
> I removed that @param tags as they are already described in the Checksum
> interface and will be picked up from there by Javadoc. Will do the same in
> CRC32 and Adler32 as well.
>
>
>> For the bounds check then you might want to look at the wording in other
>> areas (in java.lang or java.io for example) to get this better wording
>> (as off+len can overflow).
>>
> Done, I update CRC32 and Adler32 as well to make keep them as similar as
> possible.
>
>
>> A minor comment on CRC32C is that you might want to keep the line lengths
>> consistent with the rest of the code. I only mention is for future
>> side-by-side reviews to make it a bit easier and avoid too much horizontal
>> scrolling.
>>
> Done, lines are now <80 or very close in a few cases where breaking them
> made the code harder to read.
>
>>
>> -Alan
>>
>
> Here is a new webrev with the updates from Alan's and Peter's suggestions.
>     http://cr.openjdk.java.net/~sfriberg/JDK-6321472/webrev.01
>
> Thanks,
> Staffan
>
>