On Tue, 27 Sep 2022 02:40:45 GMT, ScientificWare <[email protected]> wrote:
>>> Could we reach a conciliation with this version. ? All cases are treated in
>>> 2 or 3 tests.
>>
>> Your new versions introduce one more condition to each case and make the
>> code more complicated and less clear. I am for concise, clear and readable
>> code. I am against premature optimisation, and it hasn't been proved yet
>> that there's a performance issue.
>>
>> Combining the cases of `n == 3` and `n == 4` avoids code duplication by
>> introducing one more comparison to both cases. If this becomes a critical
>> path, JIT could optimise the code.
>>
>> As such, I am still for combining these two cases. If you insist, I'm okay
>> with your original code with code duplication. Yet I'd like to eliminate the
>> duplicate code.
>>
>>> Some articles suggest to avoid using exceptions as flow controls. I never
>>> tested this point. That's why I introduced Patterns in this method.
>>
>> It's true, creating an exception object is a somewhat expensive operation.
>> Yet you say, “_most_ of color notations are correct”, therefore an incorrect
>> color which cannot be parsed is rare, so it's _an exceptional situation_, it
>> is okay to use exceptions in this case. You also say, “Pattern usage has a
>> significant time cost,” at the same time, you introduce a small time penalty
>> for each and every correct color.
>>
>> I think the hex pattern is redundant.
>>
>>> I also wish to test the version with bit rotation too :
>>>
>>> ```java
>>> return new
>>> Color(Integer.rotateRight(Integer.parseUnsignedInt(digits, 16),8),
>>> true);
>>> ```
>>
>> It could be not as clear, yet it works.
>>
>> May I suggest wrapping the code for `Color` and `rotateRight` arguments?
>>
>>
>> return new Color(
>> Integer.rotateRight(Integer.parseUnsignedInt(digits, 16),
>> 8),
>> true);
>>
>>
>> It fits better into 80-column limit. Alternatively, wrap the second
>> parameter of `rotateRight` only:
>>
>> return new
>> Color(Integer.rotateRight(Integer.parseUnsignedInt(digits, 16),
>> 8),
>> true);
>>
>> This way it's easier to see how many bits are rotated.
>
> @aivanov-jdk My previous proposition perfectly respected all your suggestions
> and didn't introduce a new test.
>
> But this discussion should be outdated if you validate this new approach.
> Performance results came from my repository I mentioned in the header.
> - Our previous codes ran in 1 200ms to 1800 ms with `String` + `formatted` +
> `%n$s` usage.
> - They ran in 350ms to 380ms with `String` + `formatted` + `%s` usage.
> - And in 100ms to 110ms if we replace `String` + `format` with a string
> concatenation.
> - Now the code below gives the same results in 45ms. Since we control
> notation length we
> - can bypass some controls,
> - directly generate the color value,
> - without generate a new string,
> - and reject a wrong number format without generate any exception.
>
>
> static final Color hexToColor(String digits) {
> int st = 0;
> if (digits.startsWith("#")) {
> st = 1;
> }
> // CSS level 4
> // - defines color hex code as #[2 digits Red][2 digits Green][2
> digits Blue][2 digits Alpha]. With digit 0 ... f.
> // - allows, webpage passes 3, 4, 6 or 8 digit color code.
> // - 3 digits #[R][G][B] ........ represents #[RR][GG][BB]FF
> // - 4 digits #[R][G][B][A] ..... represents #[RR][GG][BB][AA]
> // - 6 digits #[RR][GG][BB] ..... represents #[RR][GG][BB]FF
> // - 8 digits #[RR][GG][BB][AA] . represents #[RR][GG][BB][AA]
> //
> // Becareful ! In java.awt.Color hex #[2 digits Alpha][2 digits
> Red][2 digits Green][2 digits Blue]
> // Comme cette méthode est définie dans CSS, elle doit traiter
> uniquement le format CSS Leve 4.
> //
> // According notes below the current OpenJDK implementation is
> // - 3 digits #[R][G][B] represents #[RR][GG][BB]FF
> // - 6 digits #[R][G][B] represents #[RR][GG][BB]FF
> //
> // Some webpage passes 3 digit color code as in #fff which is
> // decoded as #000FFF resulting in blue background.
> // As per https://www.w3.org/TR/CSS1/#color-units,
> // The three-digit RGB notation (#rgb) is converted into six-digit
> form
> // (#rrggbb) by replicating digits, not by adding zeros.
> // This makes sure that white (#ffffff) can be specified with the
> short notation
> // (#fff) and removes any dependencies on the color depth of the
> display.
> byte[] idseq;
> if ((idseq = digit.get(Integer.valueOf(digits.length() - st))) ==
> null) {
> // Rejects string argument with a wrong number length.
> return null;
> }
> // Only 3, 4, 6 and 8 digits notations.
> long value = 0;
> // Parses the string argument and build color value
> for (byte i : idseq) {
> value *= 16;
> int dv = 15;
> if (i != -1) {
> if ((dv = Character.digit(digits.charAt(st + i), 16)) < 0) {
> // Rejects string argument with not a valid digit in the
> radix-16
> return null;
> }
> }
> value += dv;
> }
> return new Color((int)value, true);
> }
>
> // Index of the Digit in the Sequence
> private static Map<Integer, byte[]> digit =
> Map.ofEntries(
> Map.entry(Integer.valueOf(3), new byte[]{-1, -1, 0, 0, 1, 1, 2,
> 2}),
> Map.entry(Integer.valueOf(4), new byte[]{3, 3, 0, 0, 1, 1, 2, 2}),
> Map.entry(Integer.valueOf(6), new byte[]{-1, -1, 0, 1, 2, 3, 4,
> 5}),
> Map.entry(Integer.valueOf(8), new byte[]{6, 7, 0, 1, 2, 3, 4, 5})
> );
Should I declare `idseq` as `final` for the same reasons `a`, `b`, `c` were
declared `final` in the previous version.
-------------
PR: https://git.openjdk.org/jdk/pull/10317
