RFR JDK-6321472: Add CRC-32C API
joe darcy
joe.darcy at oracle.com
Thu Nov 20 20:34:29 UTC 2014
Hello,
Core libs changes (at this stage of JDK 9) only require a single reviewer.
Cheers,
-Joe
On 11/20/2014 4:45 AM, Staffan Friberg wrote:
> Hi,
>
> Anyone who can be the second Reviewer?
>
> Thanks,
> Staffan
>
> On 11/06/2014 04:03 PM, Staffan Friberg wrote:
>> Hi Andrej,
>>
>> Thanks for your comments. New webrev,
>> http://cr.openjdk.java.net/~sfriberg/JDK-6321472/webrev.08
>>
>> Indeed more common :)
>>
>> jdk/src/java.base/share$ grep -R "private final static" *|wc -l
>> 282
>> jdk/src/java.base/share$ grep -R "private static final" *|wc -l
>> 3274
>>
>> //Staffan
>>
>> On 11/06/2014 12:57 PM, Andrej Golovnin wrote:
>>> Hi Staffan,
>>>
>>> I'm not a reviewer but I have small remarks to the code style:
>>>
>>> - the instance variable "crc" is declared bevor the class variables.
>>> I would move it closer to the constructor.
>>>
>>> - documentation comments should be used for the fields "crc" and
>>> "CRC32C_POLY", e.g. instead of
>>>
>>> // Calculated CRC-32C value
>>> private int crc = 0xFFFFFFFF;
>>>
>>> use this:
>>>
>>> /**
>>> * Calculated CRC-32C value
>>> */
>>> private int crc = 0xFFFFFFFF;
>>>
>>>
>>> - the description of the implemented algorithm should use block
>>> comments and should be moved to the top of the class, e.g.:
>>>
>>> public final class CRC32C implements Checksum {
>>>
>>> /*
>>> * This CRC-32C implementation uses the 'slicing-by-8'
>>> algorithm described
>>> * in the paper "A Systematic Approach to Building High
>>> Performance
>>> * Software-Based CRC Generators" by Michael E. Kounavis and
>>> Frank L. Berry,
>>> * Intel Research and Development
>>> */
>>>
>>> ...
>>>
>>> In other case I get the filling that the Usafe class implements the
>>> algorithm. :-)
>>>
>>> - why some of the fields declared as "private transient final
>>> static"? I would say "transient" is not needed here. Or am I missing
>>> something? And I would use "private static final". I think this is
>>> the preferred way in the JDK to declare a constant.
>>>
>>> 61 private transient final static Unsafe UNSAFE =
>>> Unsafe.getUnsafe();
>>> 62
>>> 63 // Lookup tables
>>> 64 private transient final static int[][] byteTables = new
>>> int[8][256];
>>> 65 private transient final static int[] byteTable;
>>> 66 private transient final static int[] byteTable0 =
>>> byteTables[0];
>>>
>>>
>>> Best regards,
>>> Andrej Golovnin
>>
>
More information about the core-libs-dev
mailing list