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
