[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: RFR: JDK-8293776 : Adds CSS 4 and 8 digits hex coded Color [v6]
From:       ScientificWare <duke () openjdk ! org>
Date:       2022-09-28 21:56:30
Message-ID: IABzNOAMVAPF3uX26tLrBpNG79oPsJleWRdsaM8Mo-c=.20dcb7e6-1c39-4308-aaa9-54b589934794 () github ! com
[Download RAW message or body]

On Tue, 27 Sep 2022 02:40:45 GMT, ScientificWare <duke@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


[prev in list] [next in list] [prev in thread] [next in thread]