On Thu, 26 Jun 2025 16:11:16 GMT, Chen Liang <li...@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-o...@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
