RFR: 8356548: Avoid using ASM to parse latest class files in tests [v5]
Serguei Spitsyn
sspitsyn at openjdk.org
Wed Jul 2 21:46:46 UTC 2025
On Thu, 26 Jun 2025 16:11:16 GMT, Chen Liang <liach at openjdk.org> wrote:
>> For early eval; test by changing the ClassReader max accepted version of test ASM to 24 instead of 25
>
> Chen Liang has updated the pull request incrementally with one additional commit since the last revision:
>
> Update test/hotspot/jtreg/compiler/calls/common/InvokeDynamicPatcher.java
>
> Co-authored-by: Andrew Haley <aph-open at littlepinkcloud.com>
Thank you for making these changes!
I've looked at the `jvmti` tests only and posted some questions and nits.
test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/MissedStackMapFrames/MissedStackMapFrames.java line 82:
> 80: };
> 81: }
> 82: };
Nit: The code above is not well readable. The variable names need to be more clear, or we need some comments explaining the abbreviations. Not everyone who is reading this code is familiar with the `ClassFile` api.
`cm`: codeModel
`mth`: ?
`optSmt`: ?
test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineAnnotations.java line 88:
> 86: var cf = ClassFile.of(ClassFile.ConstantPoolSharingOption.NEW_POOL);
> 87: return cf.transformClass(cf.parse(classfileBuffer), new ClassTransform() {
> 88: // Shuffle constant pool
Q: What does `cf` mean, class file, code model or ...?
test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineGenericSignatureTest.java line 151:
> 149: var cf = ClassFile.of();
> 150: return cf.transformClass(cf.parse(bytecode), new ClassTransform() {
> 151: private boolean sourceSet = false;
Q: Same question as above.
test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineObject.java line 71:
> 69: } else {
> 70: cb.with(ce);
> 71: }
Nit: I'd suggest to rename the variables with implicit types:
`cb` => `builder` or `classBuilder`
`ce` => `element` or `classElement`
test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineRetransform/RedefineRetransform.java line 102:
> 100: var cm = ClassFile.of().parse(classBytes);
> 101: var rvaa = cm.findAttribute(Attributes.runtimeVisibleAnnotations()).orElseThrow();
> 102: var elements = rvaa.annotations().stream().filter(anno -> anno.className().isFieldType(CD_ClassVersion)).findFirst().orElseThrow().elements();
Nit: The line above is too long.
-------------
Changes requested by sspitsyn (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/25124#pullrequestreview-2980702277
PR Review Comment: https://git.openjdk.org/jdk/pull/25124#discussion_r2181014791
PR Review Comment: https://git.openjdk.org/jdk/pull/25124#discussion_r2181020007
PR Review Comment: https://git.openjdk.org/jdk/pull/25124#discussion_r2181021096
PR Review Comment: https://git.openjdk.org/jdk/pull/25124#discussion_r2181025634
PR Review Comment: https://git.openjdk.org/jdk/pull/25124#discussion_r2181026494
More information about the serviceability-dev
mailing list