-
Notifications
You must be signed in to change notification settings - Fork 5.8k
8257450: Start of release updates for JDK 17 #1531
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
Conversation
👋 Welcome back darcy! A progress list of the required criteria for merging this PR into |
@jddarcy The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
/test |
Could not create test job |
/issue 8257451 |
@jddarcy |
@jddarcy |
|
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.
Looks good - 99% sym stuff :)
Doesn't look like the hotspot deprecated flag test will need updating this time, as no newly deprecated flags marked for obsoletion in 17.
src/hotspot/share/opto/type.cpp
Outdated
const Type* mt2 = t->xmeet(this); | ||
if (mt != mt2) { | ||
tty->print_cr("=== Meet Not Commutative ==="); | ||
tty->print("t = "); t->dump(); tty->cr(); | ||
tty->print("this = "); dump(); tty->cr(); | ||
tty->print("t meet this = "); mt2->dump(); tty->cr(); | ||
tty->print("this meet t = "); mt->dump(); tty->cr(); | ||
fatal("meet not commutative"); | ||
} |
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.
Merge issue??
@@ -217,7 +223,7 @@ public Target requiredTarget() { | |||
PATTERN_MATCHING_IN_INSTANCEOF(JDK16, Fragments.FeaturePatternMatchingInstanceof, DiagKind.NORMAL), | |||
REIFIABLE_TYPES_INSTANCEOF(JDK16, Fragments.FeatureReifiableTypesInstanceof, DiagKind.PLURAL), | |||
RECORDS(JDK16, Fragments.FeatureRecords, DiagKind.PLURAL), | |||
SEALED_CLASSES(JDK16, Fragments.FeatureSealedClasses, DiagKind.PLURAL), | |||
SEALED_CLASSES(JDK17, Fragments.FeatureSealedClasses, DiagKind.PLURAL), |
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.
Is this changed because it is still preview?
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.
Right; JEP 397: "Sealed Classes (Second Preview)" is PTT for JDk 16.
@@ -24,7 +24,7 @@ | |||
|
|||
#include "precompiled.hpp" | |||
|
|||
#ifdef AARCH64 | |||
#if defined(AARCH64) && !defined(ZERO) |
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.
Another merge issue.
Webrevs
|
Usual start of release update:
One javadoc test that failed under the new version was fixed before this PR was sent out. |
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.
Build changes look good and I'm happy there are so few of them!
@jddarcy 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 no new commits pushed to the ➡️ To integrate this PR with the above commit message to the |
The langtools changes look fine to me. There were additions/changes to jcod files under |
After a merge, the tests in that directory still pass. Will keep merging in new changes and do at least more more test run before pushing. Thanks. |
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.
I've read all the files, and approve all the langtools related ones (i.e. not hotspot)
As you've noticed elsewhere, there's a pending conflict with Magnus' work to move files around.
@@ -107,7 +107,12 @@ | |||
/** | |||
* 16, tbd |
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.
The "tbd" can presumably be filled in now
{ 61, 0, Set.of()}, // JDK 17 | ||
{ 61, 0, Set.of(STATIC) }, // JDK 17 | ||
{ 61, 0, Set.of(TRANSITIVE) }, | ||
{ 61, 0, Set.of(STATIC, TRANSITIVE) }, | ||
|
||
{ 62, 0, Set.of()}, // JDK 18 |
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.
This seems unduly repetitive. Could this be dynamically generated, perhaps in a future release?
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.
I've had similar thoughts; that strikes me as a fine RFE for after this fork. I see what the code is doing, but haven't delved into the module system details to understand exactly the rationale for these tests. In any case, filed the RFE JDK-8257856: "Make ClassFileVersionsTest.java robust to JDK version updates."
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.
There was a change to JVMS 4.7.25 in Java 10 to add a rule for the requires_flags that are allowed. This is why this test started was created to test 53.0 vs. 54.0 class files. It wasn't intended to be a burden to update at each release so I'll re-implement it.
- compiler.warn.preview.feature.use.classfile: Bar.class, 16 | ||
- compiler.warn.preview.feature.use.classfile: Bar.class, 17 |
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.
Is this a test can could be automated? (i.e. no manual change per release?)
* @bug 4981566 5028634 5094412 6304984 7025786 7025789 8001112 8028545 8000961 8030610 8028546 8188870 8173382 8173382 8193290 8205619 8028563 8245147 8245586 | ||
* @bug 4981566 5028634 5094412 6304984 7025786 7025789 8001112 8028545 8000961 8030610 8028546 8188870 8173382 8173382 8193290 8205619 8028563 8245147 8245586 8257453 |
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.
long lines are annoying
@@ -293,13 +294,20 @@ protected void checksrc15(List<String> args) { | |||
printargs("checksrc15", args); | |||
expectedPass(args, List.of("New7.java", "New8.java", "New10.java", "New11.java", | |||
"New14.java", "New15.java")); | |||
// Add expectedFail after new language features added in a later release. | |||
expectedFail(args, List.of("New16.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.
(minor) looks like bad indentation
* | ||
* @since 17 | ||
*/ | ||
RELEASE_17; |
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.
Would it make sense to have a RELEASE_LATEST for the cases that are just updated to the latest release every six months?
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.
That kind of design was considered and rejected with the API was initially added. The use of enum constants in annotations must be an actual enum constant, not just a static final field pointing to a particular enum value. It would be possible to conceptually alias RELEASE_LATEST with whatever actual constant was the latest (16, then 17, then 18...), but that would cause issues with other uses of the API.
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.
Build changes (or should I say "change"?) looks good!
/integrate |
@jddarcy Since your change was applied there have been 10 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 6be1f56. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Start of JDK 17 updates.
Progress
Issues
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/1531/head:pull/1531
$ git checkout pull/1531