RFR JDK-6321472: Add CRC-32C API

[Staffan Friberg]

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