RFR: 8354329: Rewrite runtime/ClassFile/JsrRewriting.java and OomWhileParsingRepeatedJsr.java tests [v4]

[Anton Artemov]

On Fri, 2 May 2025 14:20:54 GMT, Matias Saavedra Silva <matsaave at openjdk.org> wrote:

>> Anton Artemov has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   8354329: Got rid of unused variables.
>
> test/hotspot/jtreg/runtime/ClassFile/JsrRewriting.java line 47:
> 
>> 45: import jdk.test.lib.process.OutputAnalyzer;
>> 46: 
>> 47: import java.io.File;
> 
> Imports should be clustered together in alphabetical order

Addressed in the latest commit.

> test/hotspot/jtreg/runtime/ClassFile/OOMCrashClass1960_2.java line 33:
> 
>> 31: 
>> 32: /*
>> 33: This is class is a dumper for the original OOMCrashClass1960_2.class, i.e. its dump() method
> 
> Was there a copyright date on the original version of this class? If so maybe it should be included in the header

No, there was no copyright, as this file was generated by me using ASMifier, and then refactored. I added a standard copyright comment there.

> test/hotspot/jtreg/runtime/ClassFile/OOMCrashClass1960_2.java line 63:
> 
>> 61:             methodVisitor.visitMethodInsn(INVOKESTATIC, "java/lang/Integer", "parseInt", "(Ljava/lang/String;)I", false);
>> 62:             methodVisitor.visitVarInsn(ISTORE, 1);
>> 63:             Label label1 = new Label();
> 
> Just a suggestion so you can ignore this if you prefer the current style: Declaring labels in the middle of the code is a bit confusing since it isn't actually applied until you call `visitLabel()`. Maybe it would be better to declare the labels at the top of the method.

This code was auto-generated by ASMifier, essentially I identified a repeating pattern and put to a method in order to make the code feasible in size. Declaration of a label is a part of that pattern.

> test/hotspot/jtreg/runtime/ClassFile/OomWhileParsingRepeatedJsr.java line 45:
> 
>> 43:  */
>> 44: 
>> 45: import jdk.test.lib.process.ProcessTools;
> 
> Same as above, imports should be in alphabetical order

Addressed in the latest commit.

> test/hotspot/jtreg/runtime/ClassFile/OomWhileParsingRepeatedJsr.java line 69:
> 
>> 67:         // in order to trigger and observe a fake os::malloc oom. This needs NMT.
>> 68:         ProcessBuilder pb = ProcessTools.createLimitedTestJavaProcessBuilder(
>> 69:                 "-cp", ".",
> 
> Align the arguments here

Addressed in the latest commit.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24662#discussion_r2071732255
PR Review Comment: https://git.openjdk.org/jdk/pull/24662#discussion_r2071728152
PR Review Comment: https://git.openjdk.org/jdk/pull/24662#discussion_r2071724161
PR Review Comment: https://git.openjdk.org/jdk/pull/24662#discussion_r2071732107
PR Review Comment: https://git.openjdk.org/jdk/pull/24662#discussion_r2071732387