On Mon, 26 Sep 2022 11:03:54 GMT, Alexey Ivanov <aiva...@openjdk.org> wrote:
>> Could we reach a conciliation with this version. ? All cases are treated in
>> 2 or 3 tests.
>> It incorporates all your suggestions and my single comment about the number
>> of tests.
>>
>> if (n < 5) {
>> if (n < 3) {
>> return null;
>> } else {
>> final char r = digits.charAt(0);
>> final char g = digits.charAt(1);
>> final char b = digits.charAt(2);
>> final char a = n == 4 ? digits.charAt(3) : 'f';
>> digits = String.format("%1$s%1$s%2$s%2$s%3$s%3$s%4$s%4$s",
>> r, g, b, a);
>> }
>> } else if (n < 7) {
>> if (n == 5) {
>> return null;
>> } else {
>> digits = String.format("%sff", digits);
>> }
>> } else if (n != 8 ) {
>> return null;
>> }
>> try {
>> int i = Integer.parseUnsignedInt(digits, 16);
>> return new Color((i >> 24) & 0xFF, (i >> 16) & 0xFF, (i >> 8) &
>> 0xFF, i & 0xFF);
>> } catch (NumberFormatException nfe) {
>> return null;
>> }
>>
>> Some articles suggest to avoid using exceptions as flow controls. I never
>> tested this point. That's why I introduced Patterns in this method. May I
>> run tests in this case ? With the code below :
>>
>> if (n < 5) {
>> if (n < 3 || !hex.matcher(digits).matches()) {
>> return null;
>> } else {
>> final char r = digits.charAt(0);
>> final char g = digits.charAt(1);
>> final char b = digits.charAt(2);
>> final char a = n == 4 ? digits.charAt(3) : 'f';
>> digits = String.format("%1$s%1$s%2$s%2$s%3$s%3$s%4$s%4$s",
>> r, g, b, a);
>> }
>> } else if (n < 7) {
>> if (n == 5 || !hex.matcher(digits).matches()) {
>> return null;
>> } else {
>> digits = String.format("%sff", digits);
>> }
>> } else if (n != 8 || !hex.matcher(digits).matches()) {
>> return null;
>> }
>> try {
>> int i = Integer.parseUnsignedInt(digits, 16);
>> return new Color((i >> 24) & 0xFF, (i >> 16) & 0xFF, (i >> 8) &
>> 0xFF, i & 0xFF);
>> } catch (NumberFormatException nfe) {
>> return null;
>> }
>>
>> I also wish to test the version with bit rotation too :
>>
>> if (n < 5) {
>> if (n < 3 || !hex.matcher(digits).matches()) {
>> return null;
>> } else {
>> final char r = digits.charAt(0);
>> final char g = digits.charAt(1);
>> final char b = digits.charAt(2);
>> final char a = n == 4 ? digits.charAt(3) : 'f';
>> digits = String.format("%1$s%1$s%2$s%2$s%3$s%3$s%4$s%4$s",
>> r, g, b, a);
>> }
>> } else if (n < 7) {
>> if (n == 5 || !hex.matcher(digits).matches()) {
>> return null;
>> } else {
>> digits = String.format("%sff", digits);
>> }
>> } else if (n != 8 || !hex.matcher(digits).matches()) {
>> return null;
>> }
>> try {
>> return new
>> Color(Integer.rotateRight(Integer.parseUnsignedInt(digits, 16),8),
>> true);
>> } catch (NumberFormatException nfe) {
>> return null;
>> }
>
>> 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;
if (i != -1){
int dv = Character.digit(digits.charAt(st + i),16);
if (dv < 0) {
// Rejects string argument with not a valid digit in the
radix-16
return null;
}
value += dv;
} else {
value += 15;
}
}
return new Color((int)value, true);
}
// Index of the Digit in the Sequence -1 means f.
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})
);
-------------
PR: https://git.openjdk.org/jdk/pull/10317
