-
Couldn't load subscription status.
- Fork 6.1k
8335991: Implement Simple Source Files and Instance Main Methods (Fourth Preview) #21787
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 jlahoda! A progress list of the required criteria for merging this PR into |
|
@lahodaj 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 299 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
/jep 495 |
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 to me.
| //--- | ||
| @JEP(number=477, title="Implicitly Declared Classes and Instance Main Methods", status="Third Preview") | ||
| @JEP(number=495, title="Simple Source Files and Instance Main Methods", status="Fourth Preview") | ||
| IMPLICIT_CLASSES, |
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.
Hello Jan, although it's just an internal enum name, do you think we should rename it to be closer to the JEP's title or is it not worth it?
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 title also shows up in list of preview APIs in the javadoc so changing seems correct.
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.
Please note the title is adjusted to match the current name of the JEP. So the built javadoc should not be an issue. What Jaikiran is speaking about is the enum constant name, which hopefully(!) does not have a meaning outside of the JDK.
We could change the constant to SIMPLE_SOURCE_FILES_AND_INSTANCE_MAIN_METHODS (to precisely reflect the JEP's name), but it seems like an unnecessary git churn to me. We might also need to keep the existing constant due to bootstrap issues, then remove it when JDK 25 is switched to bootstrap on JDK 24.
I can do that, if really needed, though.
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.
What Jaikiran is speaking about is the enum constant name, which hopefully(!) does not have a meaning outside of the JDK.
I had looked around the rendered javadoc and a few other places and I didn't notice the enum name showing up anywhere publicly. It merely appears to be an internal reference within the code.
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 looks OK to me and matches what is stated in the CSR.
|
/integrate |
|
Going to push as commit 5731ab7.
Your commit was automatically rebased without conflicts. |
This is a partial implementation of JEP 495 - adjustments of the JEP metadata in
PreviewFeature. There are no language changes associated with this JEP. Changes to thejava.io.IOclass are covered by #21693.Progress
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/21787/head:pull/21787$ git checkout pull/21787Update a local copy of the PR:
$ git checkout pull/21787$ git pull https://git.openjdk.org/jdk.git pull/21787/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 21787View PR using the GUI difftool:
$ git pr show -t 21787Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21787.diff
Using Webrev
Link to Webrev Comment