I wouldn't even bother with ENDIAN field; ByteOrder.nativeOrder(), which calls Bits.byteOrder(), which returns a static final field (modulo a null check) should get JIT compiled into just a read of Bits.byteOrder. If storing ByteOrder.nativeOrder() in a static final field actually makes a difference vs using ByteOrder.nativeOrder() in JIT compiled code, I'd be pretty sad :).
On Fri, Oct 17, 2014 at 3:45 PM, Stanimir Simeonoff <[email protected]> wrote: > 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 < > [email protected] > > 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 > > > > >
