RFR: JDK-8293776 : Adds CSS 4 and 8 digits hex coded Color [v6]
ScientificWare
duke at openjdk.org
Wed Sep 28 22:01:12 UTC 2022
On Tue, 27 Sep 2022 02:40:45 GMT, ScientificWare <duke at openjdk.org> 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
More information about the client-libs-dev
mailing list