RFR: 8229186: Improve error messages for TestStringIntrinsics failures [v5]

[Igor Ignatyev]

On Wed, 30 Sep 2020 20:20:08 GMT, Evgeny Nikitin <enikitin at openjdk.org> wrote:

>>> The other method, `invokeAndCheck` is different. It can call, for example, `String.concat("abc", "def")` and expect the
>>> String "abcdef" as result. It does really need to be generic.
>> although I still don't see them as really being that much different, I'm fine w/ keeping them as-is.
>> 
>> 
>>> `invokeAndCompareArrays` / `ArrayDiff` compares two huge arrays and present a nice short _slice_ in the difference
>>> area. The short slice is possible because we do a limited task - compare arrays. `invokeAndCheck` , on the other hand,
>>> can have as parameters the `utf16`, a string of 10K symbols. Do we really need a 10K string as output in our log?
>> 
>> I think we do, as people tend to first look at exception's messages and only then look thru other logs, so having
>> relative information in the exception is always a good thing. should 10K symbols become a problem (this though would
>> also mean that you can't print the compared values w/  `Format.asLiteral`), we can revisit this.
>>> 
>>> > you don't need `ArrayCodec::of(Object array)` anymore, do you?
>>> 
>>> Unfortunately, it is used in the ArrayCodec.format (which is used in TestStringIntrinsics.java) - to make it possible
>>> to call it with everything and not swamp the code with overloads.
>> 
>> hm, I somehow missed that usage, but you don't need to repeat to the same switch over a component type in
>> `ArrayDiff::of`, do you?
>
>> I think we do, as people tend to first look at exception's messages and only then look thru other logs, so having
>> relative information in the exception is always a good thing. should 10K symbols become a problem (this though would
>> also mean that you can't print the compared values w/ `Format.asLiteral`), we can revisit this.
> 
> Ok, I got it. I was blind and stupid, sorry. Fixed, please check the e6fb6d04cad
> 
>> hm, I somehow missed that usage, but you don't need to repeat to the same switch over a component type in
>> `ArrayDiff::of`, do you?
> 
> <sigh>... I can see no better solution here. The `ArrayDiff::of` checks that both component types are the same in
> addition, but I probably could have avoided duplicating the switch (we can ignore double guessing the type). The issue
> is that <E> in ArrayDiff is forwarded to first and second, I would need to declare them as generic
> (ArrayCodec<Object>). The type system seem to not allow me to fix that nicely.

> > I think we do, as people tend to first look at exception's messages and only then look thru other logs, so having
> > relative information in the exception is always a good thing. should 10K symbols become a problem (this though would
> > also mean that you can't print the compared values w/ `Format.asLiteral`), we can revisit this.
> 
> Ok, I got it. I was blind and stupid, sorry. Fixed, please check the
> [e6fb6d0](https://github.com/openjdk/jdk/commit/e6fb6d04caddc4b410a594574f570cfcb9445e4a)

I'd use `StringBuilder` to construct the message.

> > hm, I somehow missed that usage, but you don't need to repeat to the same switch over a component type in
> > `ArrayDiff::of`, do you?
>
> <sigh>... I can see no better solution here. The `ArrayDiff::of` checks that both component types are the same in
> addition, but I probably could have avoided duplicating the switch (we can ignore double guessing the type). The issue
> is that in ArrayDiff is forwarded to first and second, I would need to declare them as generic (ArrayCodec). The type
> system seem to not allow me to fix that nicely.

wouldn't the following patch do?


 --git a/test/lib/jdk/test/lib/format/ArrayDiff.java b/test/lib/jdk/test/lib/format/ArrayDiff.java
--- a/test/lib/jdk/test/lib/format/ArrayDiff.java
+++ b/test/lib/jdk/test/lib/format/ArrayDiff.java
@@ -123,41 +123,10 @@ public class ArrayDiff<E> implements Diff {
         if (!bothAreArrays || !componentTypesAreSame) {
             throw new IllegalArgumentException("Both arguments should be arrays of the same type");
         }
-
-        var type = first.getClass().getComponentType();
-        if (type == byte.class) {
-            return new ArrayDiff<>(
-                    ArrayCodec.of((byte[])first),
-                    ArrayCodec.of((byte[])second),
-                    width, contextBefore);
-        } else if (type == int.class) {
-            return new ArrayDiff<>(
-                    ArrayCodec.of((int[])first),
-                    ArrayCodec.of((int[])second),
-                    width, contextBefore);
-        } else if (type == long.class) {
-            return new ArrayDiff<>(
-                    ArrayCodec.of((long[])first),
-                    ArrayCodec.of((long[])second),
-                    width, contextBefore);
-        } else if (type == char.class) {
-            return new ArrayDiff<>(
-                    ArrayCodec.of((char[])first),
-                    ArrayCodec.of((char[])second),
-                    width, contextBefore);
-        } else if (type == String.class) {
-            return new ArrayDiff<>(
-                    ArrayCodec.of((String[])first),
-                    ArrayCodec.of((String[])second),
-                    width, contextBefore);
-        } else if (!type.isPrimitive() && !type.isArray()) {
-            return new ArrayDiff<>(
-                    ArrayCodec.of((Object[])first),
-                    ArrayCodec.of((Object[])second),
-                    width, contextBefore);
-        }
-
-        throw new IllegalArgumentException("Unsupported array component type: " + type);
+        return new ArrayDiff(
+                ArrayCodec.of(first),
+                ArrayCodec.of(second),
+                width, contextBefore);
     }
 
     /**

-------------

PR: https://git.openjdk.java.net/jdk/pull/112