On Wed, 15 May 2024 19:34:40 GMT, Volodymyr Paprotski <d...@openjdk.org> wrote:
>> Scott Gibbons has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Rearrange; add lambdas for clarity
>
> test/jdk/java/lang/StringBuffer/IndexOf.java line 90:
>
>> 88:
>> 89: // printStringBytes(shs.getBytes(hs_charset));
>> 90: for (int i = 0; i < 200000; i++) {
>
> This wont be a deterministic way to reach the intrinsic. I would suggest
> copying the idea from
> test/jdk/com/sun/crypto/provider/Cipher/ChaCha20/unittest/Poly1305UnitTestDriver.java
>
> i.e. Have two `@run main` invocations at the top of this file, one with
> default parameters, one with `-Xcomp -XX:-TieredCompilation`. You dont need
> a 'driver' program, that was to handle something else.
>
>
> /*
> * @test
> * @modules java.base/com.sun.crypto.provider
> * @run main java.base/com.sun.crypto.provider.Poly1305KAT
> * @summary Unit test for com.sun.crypto.provider.Poly1305.
> */
>
> /*
> * @test
> * @modules java.base/com.sun.crypto.provider
> * @summary Unit test for IntrinsicCandidate in
> com.sun.crypto.provider.Poly1305.
> * @run main/othervm -Xcomp -XX:-TieredCompilation
> -XX:+UnlockDiagnosticVMOptions -XX:+ForceUnreachable
> java.base/com.sun.crypto.provider.Poly1305KAT
> */
Done.
> test/jdk/java/lang/StringBuffer/IndexOf.java line 126:
>
>> 124: int aNewLength = getRandomIndex(min, max);
>> 125: for (int y = 0; y < aNewLength; y++) {
>> 126: int achar = generator.nextInt(30) + 30;
>
> This will only ever generate LL cases, i.e. chars from [30,60]. Could be
> parametrized to also produce utf16 if instead of 30, offset was in the
> unicode range
Original code.
> test/jdk/java/lang/StringBuffer/IndexOf.java line 199:
>
>> 197:
>> System.out.println("Source="+sourceString.substring(hsBegin, hsBegin +
>> haystackLen));
>> 198:
>> System.out.println("Target="+targetString.substring(nBegin, nBegin +
>> needleLen));
>> 199: System.out.println("haystackLen="+haystackLen+"
>> neeldeLen="+needleLen+" hsBegin="+hsBegin+" nBegin="+nBegin+
>
> This looks like 'development scaffolding' (i.e. printf debugging) that was
> meant to be removed
This is additional information printed upon failure instead of just saying
"failed"
> test/jdk/java/lang/StringBuffer/IndexOf.java line 295:
>
>> 293: sourceString = generateTestString(99, 100);
>> 294: sourceBuffer = new StringBuffer(sourceString);
>> 295: targetString = generateTestString(10, 11);
>
> Generate a random int [0,1,2] for LL, UU, UL, pass that as parameter to
> generateTestString() to test the other paths. Same for other tests in this
> file using this pattern.
>
> This test is specific to haystacklen=100, needlelen=10.. what about other
> haystack/needle sizes to exercise all the paths in the intrinsic assembler
> (i.e. haystack >=, <=32, needlelen ={1,2,3,4,5..32..}). Elsewhere already?
Original code.
> test/jdk/java/lang/StringBuffer/IndexOf.java line 360:
>
>> 358: System.err.println(" sAnswer = " + sAnswer + ", sbAnswer = " +
>> sbAnswer);
>> 359: System.err.println(" testString = '" + testString + "'");
>> 360: System.err.println(" testBuffer = '" + testBuffer + "'");
>
> tracing left here and further down
Adding more information on failure.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1613915508
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1613919180
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1613920449
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1613922554
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1613923075
