On Wed, 1 Jul 2026 11:31:57 GMT, Alan Bateman <[email protected]> wrote:
>> Chen Liang has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains six additional >> commits since the last revision: >> >> - Merge branch 'lworld' of https://github.com/openjdk/valhalla into >> doc/lw-reflect-preview-api-0 >> - Merge branch 'lworld' of https://github.com/openjdk/valhalla into >> doc/lw-reflect-preview-api-0 >> - Merge branch 'lworld' of https://github.com/openjdk/valhalla into >> doc/lw-reflect-preview-api-0 >> - Reword recommendations >> - Make requireIdentity reflective >> - Reflective preview API behavior out of preview > > src/java.base/share/classes/java/lang/Class.java line 1411: > >> 1409: * enabled, a status not accessible to programs. The >> preferred way >> 1410: * to check for the identity status is to use {@link >> #isValue() >> 1411: * Class.isValue()}. > > I think it would be better to drop "a status not accessible to programs", it > will just anger developers. > > Can the second sentence just use "Use the isValue method to test if a value > class or identity class"? “a status not accessible to programs” is not even true, as user code can just try to load a hidden preview class for the current `ClassFileFormatVersion` and check whether it succeeds, like what the `defineHiddenClass/PreviewHiddenClass` **JDK** and `ClassFile/PreviewVersion` **JTReg** tests do: // `@Namespace` is an internal annotation that signals that the class is intended // to be used to only hold `static` fields and methods public final @Namespace class PreviewFeatures { private PreviewFeatures() throws InstantiationException { throw new InstantiationException(PreviewFeatures.class.getName()); } /// {@return whether preview features are enabled} public static boolean isEnabled() { return Holder.IS_PREVIEW_ENABLED; } // Can be replaced with `LazyConstant` once that exits preview private static final @Namespace class Holder { private Holder() throws InstantiationException { throw new InstantiationException(Holder.class.getName()); } private static final boolean IS_PREVIEW_ENABLED = computeIsPreviewEnabled(); } private static boolean computeIsPreviewEnabled() { final var lookup = MethodHandles.lookup(); final byte[] bytecodes = getCurrentPreviewBytecodes(); try { lookup.defineHiddenClass(bytecodes, false, new MethodHandles.Lookup.ClassOption[] { // no `ClassOption.STRONG`, so that the temporary hidden class // can be collected after this method exits }); return true; } catch (final UnsupportedClassVersionError _) { return false; } catch (final IllegalAccessException ex) { // `ExceptionUtil.fromReflectiveOperationException` behaves like // `java.lang.invoke.MethodHandleNatives::mapLookupExceptionToError` throw ExceptionUtil.fromReflectiveOperationException(ex); } } /// {@return the minimum bytecode needed to check whether preview features are enabled} private static final byte[] getCurrentPreviewBytecodes() { return ClassFile.of().build( ClassDesc.of( PreviewFeatures.class.getName() .replace('/', '$$') .concat("$PreviewCheck") ), cb -> cb .withVersion( ClassFileFormatVersion.latest().major(), ClassFile.PREVIEW_MINOR_VERSION ) .withFlags(ACC_PUBLIC | ACC_FINAL | ACC_SUPER) .withSuperclass(ConstantDescs.CD_Object) ); } } > src/java.base/share/classes/java/util/Objects.java line 238: > >> 236: * @since 28 >> 237: */ >> 238: @PreviewFeature(feature = PreviewFeature.Feature.VALUE_OBJECTS, >> reflective = true) > > Changing the requireIdentity methods to be reflective preview APIs looks > okay, as is the update to their docs. My guess is that the libraries that > want to get a head start will access the preview APIs with core reflection so > they won't care if the methods are just normal preview APIs. For a similar reason, `IdentityException` should probably also be made reflective. ------------- PR Review Comment: https://git.openjdk.org/valhalla/pull/2578#discussion_r3513445559 PR Review Comment: https://git.openjdk.org/valhalla/pull/2578#discussion_r3512069592
