Further review of java.util.HexFormat
Roger Riggs
roger.riggs at oracle.com
Wed Mar 31 13:53:54 UTC 2021
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
More information about the core-libs-dev
mailing list