On Mon, 11 May 2026 14:22:45 GMT, David Simms <[email protected]> wrote:
> This is a "*sub-review pull request*" for the first > [preview](https://openjdk.org/jeps/12) of [JEP 401: Value Classes and > Objects](https://openjdk.org/jeps/401), specifically > [JDK-8317279](https://bugs.openjdk.org/browse/JDK-8317279): Standard library > implementation of value classes and objects > >> [!NOTE] >> This pull request and the other sub-review pull requests listed below are >> based on the "*master pull request*" >> (https://github.com/openjdk/jdk/pull/31120). It contains the same full set >> of code changes as the "*master pull request*" to preserve the full >> implementation context; the language compiler, JVM, and standard library >> changes are intertwined. This separate pull requests exist only to subdivide >> the review and related discussion by area. > > Other areas for review: > > - [JDK-8317277](https://bugs.openjdk.org/browse/JDK-8317277): Java language > implementation of value classes and objects > - https://github.com/openjdk/jdk/pull/31121 > - [JDK-8317278](https://bugs.openjdk.org/browse/JDK-8317278): JVM > implementation of value classes and objects > - https://github.com/openjdk/jdk/pull/31122 > > Code changes resulting from the review process should be made in > [`valhalla/lworld`](https://github.com/openjdk/valhalla/). > > `valhalla/lworld` is currently updated from `jdk/master` whenever a weekly > [`jdk` tag](https://github.com/openjdk/jdk/tags) is created. At that time, > code changes from `valhalla/lworld` will be propagated to the master pull > request and to all sub-review pull requests, including this one. > > Ultimately, review sign-off will be recorded on the "*master pull request*", > and this pull request will be closed without integration. > > This pull request has a large code surface area and often conflicts with > `jdk/master` on a daily basis. Refer to > [`valhalla/lworld`](https://github.com/openjdk/valhalla/) for the latest > state of the project code, keeping in mind that it may lag several days > behind `jdk/master`. Both repositories may be needed as references during > review. > > --------- > - [x] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). Mostly style and code quality issues reported. .github/workflows/main.yml line 177: > 175: echo 'true' > 176: return > 177: fi This looks like left over project specific logic that should probably be removed before integration into the jdk repo. make/CompileJavaModules.gmk line 142: > 140: PREVIEW_PATH := META-INF/preview > 141: > 142: ifneq ($(COMPILER), bootjdk) This conditional looks out of place. There is nothing setting `COMPILER` in this file and there is nothing in the call chain to this file. What is this trying to protect against? Suggestion: make/CompileJavaModules.gmk line 150: > 148: # We cannot compile directly into the desired directory because it's > the > 149: # compiler which creates the original '<module>/<classpath>/...' > hierarchy. > 150: TEMP_OUTPUTDIR := $(SUPPORT_OUTPUTDIR)/$(PREVIEW_CLASSES_LABEL) Avoid the word "temporary" when describing this procedure. The files aren't removed after copying (and they shouldn't be), a better word describing these class files or the directory they are compiled into would be "intermediate", or just name them for what they are "preview". In this case, just remove the word "temporary" from the description. The variable name should also be changed. Suggestion: # Compile preview classes into a separate directory, and then copy into the # correct output path location. We cannot compile directly into the desired # directory because it's the compiler which creates the original # '<module>/<classpath>/...' hierarchy. PREVIEW_OUTPUTDIR := $(SUPPORT_OUTPUTDIR)/$(PREVIEW_CLASSES_LABEL) make/CompileJavaModules.gmk line 158: > 156: INCLUDES := $(JDK_USER_DEFINED_FILTER), \ > 157: FAIL_NO_SRC := $(FAIL_NO_SRC), \ > 158: BIN := $(TEMP_OUTPUTDIR)/, \ Suggestion: BIN := $(PREVIEW_OUTPUTDIR)/, \ make/CompileJavaModules.gmk line 159: > 157: FAIL_NO_SRC := $(FAIL_NO_SRC), \ > 158: BIN := $(TEMP_OUTPUTDIR)/, \ > 159: HEADERS := $(SUPPORT_OUTPUTDIR)/headers, \ This seems risky. Do we anticipate ever needing to generate JNI headers form value classes? If so, they should probably be redirected to a separate directory too. I would recommend just not configure header generation. Suggestion: make/CompileJavaModules.gmk line 177: > 175: # result we care about. > 176: > 177: # Copy compiled output from "$TEMP_OUTPUTDIR/$MODULE/<classpath>/..." Suggestion: # Copy compiled output from "$PREVIEW_OUTPUTDIR/$MODULE/<classpath>/..." make/RunTests.gmk line 212: > 210: TEST_MODE ASSERT VERBOSE RETAIN TEST_THREAD_FACTORY > JVMTI_STRESS_AGENT \ > 211: MAX_MEM RUN_PROBLEM_LISTS RETRY_COUNT REPEAT_COUNT MAX_OUTPUT > REPORT \ > 212: AOT_JDK MANUAL VALUE_CLASS_PLUGIN > $(CUSTOM_JTREG_SINGLE_KEYWORDS), \ When adding a new option for testing, please add documentation describing what it is and what it does in doc/testing.[md|html]. It's pretty obvious this has to do with value classes testing, but what does using the plugin actually do? Is the option name `VALUE_CLASS_PLUGIN` well suited for the purpose? By that I mean, is the important part that we are using a plugin, or that we are testing value classes, or something else? make/RunTests.gmk line 889: > 887: $1_JTREG_BASIC_OPTIONS += -vmoption:--enable-preview > 888: $1_JTREG_BASIC_OPTIONS += -javacoption:-XDaccessInternalAPI > 889: $1_JTREG_BASIC_OPTIONS += -javacoption:--source > -javacoption:$(VERSION_FEATURE) Is this really necessary? Wouldn't the current feature version be the default value for `--source`, or is there some interaction with --enable-preview that requires it to be paired with `--source`? make/autoconf/boot-jdk.m4 line 447: > 445: JVM_HEAP_LIMIT_32="768" > 446: # Running a 64 bit JVM allows for and requires a bigger heap > 447: JVM_HEAP_LIMIT_64="3200" This is probably ok, but I'm curious what prompted it and if that is still relevant after all the iterations I assume this project has gone through. make/conf/version-numbers.conf line 43: > 41: DEFAULT_JDK_SOURCE_TARGET_VERSION=27 > 42: DEFAULT_PROMOTED_VERSION_PRE=ea > 43: Spurious whitespace change. make/hotspot/lib/JvmFeatures.gmk line 56: > 54: JVM_EXCLUDE_FILES += templateInterpreter.cpp > templateInterpreterGenerator.cpp \ > 55: bcEscapeAnalyzer.cpp ciTypeFlow.cpp > macroAssembler_common.cpp > 56: JVM_CFLAGS_FEATURES += -DZERO > -DZERO_LIBARCH='"$(OPENJDK_TARGET_CPU_LEGACY_LIB)"' $(LIBFFI_CFLAGS) Style nits. https://openjdk.org/groups/build/doc/code-conventions.html Specifically rule 18, avoid padding to align. We also try to keep things within 80 when feasible for easier diff and merge views. This change is explicitly reformatting away a line break to make the line longer, though the old line break was certainly suboptimal. make/modules/java.base/gensrc/GensrcValueClasses.gmk line 29: > 27: # Generate the value class replacements for selected java.base source > files > 28: > 29: java.base-VALUE_CLASS-REPLACEMENTS := \ Style nit. Unconventional variable name format. It doesn't hurt but it kind of sticks out. Same below. make/modules/java.base/gensrc/GensrcVarHandles.gmk line 44: > 42: VARHANDLE_$1_type := $$(strip $$(if $$(filter > $(VARHANDLE_OBJECT_TYPES), $1), Object, $1)) > 43: VARHANDLE_$1_InputType := $$(strip $$(if $$(filter > $(VARHANDLE_OBJECT_TYPES), $1), $1, $$(call Conv, $1, Type))) > 44: VARHANDLE_$1_Type := $$(strip $$(subst NonAtomicReference, Reference, > $$(subst NonAtomicFlatValue, FlatValue, $$(VARHANDLE_$1_InputType)))) Style nit. Please try to break these lines in a readable way to avoid excessive width. Use the `#` lines for a visual guide. Going over a little is ok if it improves readability. make/test/BuildJtregValueClassPlugin.gmk line 36: > 34: # the META-INF service descriptor, compiled > WITH > 35: # --enable-preview and the required > internal-API > 36: # exports. This comment formatting is awkward. I don't think this table layout improves readability. make/test/BuildTestLib.gmk line 65: > 63: HEADERS := $(TEST_LIB_SUPPORT)/test-lib_headers, \ > 64: JAR := $(TEST_LIB_SUPPORT)/test-lib.jar, \ > 65: DISABLED_WARNINGS := try deprecation rawtypes unchecked serial cast > removal preview restricted varargs dangling-doc-comments, \ Break line, but maybe also consider improving the code quality of the new test lib classes? That's a lot of disabled warning classes. ------------- PR Review: https://git.openjdk.org/jdk/pull/31123#pullrequestreview-4337217031 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3281432370 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3283923638 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3283969521 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3283974495 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3283984821 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3284009037 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3284110599 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3284088702 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3281465745 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3281475308 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3281504350 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3281560346 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3281532327 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3282801141 PR Review Comment: https://git.openjdk.org/jdk/pull/31123#discussion_r3282816092
