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/6759a25...
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