On Mon, 19 Sep 2022 16:36:00 GMT, Alexey Ivanov <aiva...@openjdk.org> wrote:
>> ScientificWare has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Corrects typos and digit extraction.
>>
>> Corrects typos.
>> And applies a better digit extraction as suggested in main-line review
>> thread.
>
> src/java.desktop/share/classes/javax/swing/text/html/CSS.java line 1366:
>
>> 1364: digits = digits.substring(1, Math.min(n, 9));
>> 1365: n--;
>> 1366: }
>
> Should the method return `null` immediately if `n < 3 || n > 8` after the if
> block at lines 1363-1366? It's already known the format is invalid.
>
> I also propose adding `hex.matcher(digits).matches()` here to avoid running
> the matcher at each attempt.
>
> In short, the next statement should be:
>
>
> if (n < 3 || n == 7 || n > 8 || !hex.matcher(digits).matches()) {
> return null;
> }
>
>
> The following code deals with a valid color notation.
> @aivanov-jdk, I suppose that most of color notations are correct.
Likely, otherwise the colors wouldn't show up.
> Adding these tests in first place has a time cost.
The time is rather negligible, in my opinion. Yet the code is clearer.
If you care about micro-optimisations, the `n == 6` case should be the first
one in the chain of `if`-blocks as the most common case.
> With `&&`, `hex.matcher(digits).matches()` is evaluated only once. That why I
> separated length test from hex test.
Yes, you're absolutely right. I agree it is evaluated only once, which wasn't
apparent to me from the start. Someone else may also consider it as
inefficiency.
> Pattern usage has a significant time cost.
Exactly. Is it even needed? The previous code did well without it.
I'm sure it's not needed, `Integer.parseUnsignedInt` validates the digits are
valid, if not, it throws `NumberFormatException`. Since, as you say, it's rare
that the color notation is invalid, I see no reason to avoid the exception
using the pattern.
-------------
PR: https://git.openjdk.org/jdk/pull/10317
