On Mon, 1 Sep 2025 11:01:20 GMT, Maurizio Cimadamore <[email protected]>
wrote:
>> Chen Liang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Years
>
> src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line
> 348:
>
>> 346: .build(proxyDesc, clb -> {
>> 347: clb.withSuperclass(CD_Object)
>> 348: .withFlags(ACC_SUPER | ACC_FINAL | ACC_SYNTHETIC)
>
> I presume this change is because the classfile API will be able to filter out
> ACC_SUPER based on the target version?
No, because the old code is wrong - the flags are no-ops if the class file
version is not latest release's preview (by default it is latest release.0)
> src/java.base/share/classes/java/lang/reflect/AccessFlag.java line 461:
>
>> 459: * @jvms 4.1 The {@code ClassFile} Structure
>> 460: */
>> 461: CLASS(ACC_PUBLIC | ACC_FINAL | ACC_IDENTITY |
>
> Shouldn't we have a mapping here for `JDK N - 1` that has ACC_SUPER instead
> of ACC_IDENTITY ? For inner classes this is not needed because inner classes
> could never have ACC_SUPER -- but regular classes could?
This is just the bit masks, where ACC_SUPER is equivalent to ACC_IDENTITY. So
0x20 is always there in classes, but newly introduced to inner classes, so yes.
> src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/ClassReader.java line
> 3348:
>
>> 3346: flags |= MODULE;
>> 3347: }
>> 3348: if (((flags & ACC_IDENTITY) != 0 &&
>> !isMigratedValueClass(flags))
>
> do we still need the `isMigratedValueClass` ? Aren't migrated value classes
> treated the same as ordinary value classes thanks to the work we did to load
> different classfiles based on preview-ness?
I only adjusted this because this is incorrectly marking Java 25 classes as
value classes in InnerClasses. This call didn't affect my patch, should I
investigate it now?
> src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/ClassWriter.java line
> 1772:
>
>> 1770: } else {
>> 1771: poolbuf.appendChar(target.minorVersion);
>> 1772: markedPreview = target.minorVersion ==
>> ClassFile.PREVIEW_MINOR_VERSION;
>
> I don't think `target.minorVersion` can ever be preview?
Sure. I thought this path could have been used by -XDforcePreview. Do you think
I can just do:
boolean markedPreview = preview.isEnabled() &&
preview.usesPreview(c.sourcefile);
if (markedPreview) {
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1533#discussion_r2317278677
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1533#discussion_r2317276520
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1533#discussion_r2317273110
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1533#discussion_r2317270357
