-
Notifications
You must be signed in to change notification settings - Fork 107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8327636: [lworld] Make primitive wrappers be value class if preview feature is enabled #1040
Conversation
…eature is enabled
👋 Welcome back mchung! A progress list of the required criteria for merging this PR into |
@mlchung This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 2 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://bugs.openjdk.org/browse/JDK-8327481 can be closed as a duplicate of this.
java/lang/Boolean.java \ | ||
java/lang/Character.java \ | ||
java/lang/Number.java \ | ||
java/lang/Record.java \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Record is already included in the end of the list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Fixed.
@@ -208,6 +208,13 @@ public static boolean isClassAccessible(Class<?> refc, | |||
return true; | |||
} | |||
|
|||
// exports are not setup during early VM initialization | |||
if (!jdk.internal.misc.VM.isModuleSystemInited()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we merge this and the lookupModule == null
early startup cases, and assert lookupModule == refModule && refModule == Object.class.getModule()
can become assert lookupModule == refModule && (refModule == null || refModule == Object.class.getModule())
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be simpler:
// early VM startup case, java.base not defined or module system not initialized
if (lookupModule == null || !jdk.internal.misc.VM.isModuleSystemInited()) {
assert lookupModule == refModule;
return true;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can Class::getModule
ever return null
after module system is initialized? If not we can omit the lookupModule == null
check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can Class::getModule ever return null after module system is initialized?
No.
lookupModule == null
when module system is being initialized before java.base is defined. So the null check needs to be there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/integrate |
Going to push as commit 3ee4244.
Your commit was automatically rebased without conflicts. |
As specified in JEP 401, when preview features are enabled, the following standard library classes are considered to be value classes:
These value classes are loaded only if --enable-preview is set. All valhalla tests should have preview enabled to ensure testing with the proper set of library classes.
Testing: Run
jdk_valhalla
andhotspot_valhalla_runtime
with-Xint
with and without--enable-preview
. The result is pretty good. JDK-8327637 and JDK-8327639 need further investigation.Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1040/head:pull/1040
$ git checkout pull/1040
Update a local copy of the PR:
$ git checkout pull/1040
$ git pull https://git.openjdk.org/valhalla.git pull/1040/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1040
View PR using the GUI difftool:
$ git pr show -t 1040
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1040.diff
Webrev
Link to Webrev Comment