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
