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."
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.
Should CRC32C be final unless we have a good reason for it not to be final?
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.
I think it's more usual to list the @param tags before the @throws.
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).
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.
-Alan.
