Further review of java.util.HexFormat

[Roger Riggs]

Hi Raffaello,

None of these are substantive but useful none the less.
I would have rather seen them during the review and not be revisiting them.

Issue created: 8264514 
<https://bugs.openjdk.java.net/browse/JDK-8264514> HexFormat 
implementation tweaks

On 3/30/21 8:19 PM, Raffaello Giulietti wrote:
> Hello Roger,
>
>
> these are the changes I'm proposing after reviewing the code of 
> j.u.HexFormat. The modified code available here
> https://urldefense.com/v3/__https://github.com/rgiulietti/jdk/commit/6759a25eb952ab19a045a83349d41b82cc1b07cb__;!!GqivPVa7Brio!JLXpQq2CQ_x4RuLDCYWukMvEBq8yc6hUH8q7U0stTiQjEgvx6yn_h_2gwUMfmMgI$ 
>
> In addition to other smaller, hopefully self-explanatory enhancements, 
> here are the rationales for the changes.
>
>
> Static field DIGITS should preferably be formatted with 16 values/line 
> to ease a visual positional crosscheck with published ASCII/IsoLatin1 
> tables.
yes, but as is are easier to index using decimal values.  (20 per line 
and more compact).
>
>
> Field digits is initialized with either UPPERCASE_DIGITS, 
> LOWERCASE_DIGITS or digits from another instance, so it always ends up 
> being either UPPERCASE_DIGITS or LOWERCASE_DIGITS.
> Consequently:
> * There's no need for requireNonNull() check in the (sole) private 
> constructor.
> * It's preferable to move the last comparison in method equals() as 
> the first factor in the return statement, so it can return faster in 
> case of a lower/upper mismatch. (Arrays.equals() first checks for ==, 
> so it always returns fast as used in this class. It could even be 
> replaced by a simple == )
Though perhaps less intuitive and not performance sensitive.
>
>
> Method fromHexDigits(CharSequence, int) either returns a value in the 
> range [0x00..0xff] or throws.
> Therefore, there's no need for the checks leading to the throwing of 
> IllegalArgumentException in methods
> * parseHex(CharSequence, int, int)
> * parseNoDelimiter(CharSequence)
> which can be simplified as a consequence.
The private fromHexDigits method did not originally throw.
An @throws ILE should be added
>
>
> The test for IllegalArgumentException in method parseHex(CharSequence, 
> int, int), namely
> string.length() < valueChars || (string.length() - valueChars) % 
> stride != 0
> can be simplified as
> (string.length() - valueChars) % stride != 0
>
> Indeed, at this point in the control flow we have
> string.length() > 0  and  stride >= valueChars
> Assuming string.length() < valueChars as in the left operand of || we 
> then have
> -stride <= -valueChars < string.length() - valueChars < 0
> so
> string.length() - valueChars) % stride != 0
> which is the right operand of ||.
> In other words, the left operand always implies the right one, adding 
> nothing to it.
>
>
> There's no need to check again for non-nullness in private method 
> fromHexDigits(CharSequence, int). It is invoked from two places where 
> the check is already performed.
Though the checking seems redundant it documents the requirement.
The hotspot compiler squashes out any redundancy, so there is no 
performance impact.
>
>
> Both fromHexDigits(CharSequence) and fromHexDigitsToLong(CharSequence) 
> can simply invoke their 3 args counterparts.
At a slightly higher overhead to add the offset.
>
>
> If you prefer, I can prepare a PR once there's an issue in the bug 
> system to associate the PR with.
>

Thanks, Roger

>
> Greetings
> Raffaello