8265130: Make ConstantDesc class hierarchy sealed #4135

GavinBierman · 2021-05-20T21:07:04Z

Hi all,

Please review this patch to make the ConstantDesc hierarchy sealed, as was promised in its Javadoc, now that sealed classes are finalising in JDK 17.

Thanks,
Gavin

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8265130: Make ConstantDesc class hierarchy sealed

Reviewers

Mandy Chung (@mlchung - Reviewer)
Jorn Vernee (@JornVernee - Reviewer)
Vicente Romero (@vicente-romero-oracle - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4135/head:pull/4135
$ git checkout pull/4135

Update a local copy of the PR:
$ git checkout pull/4135
$ git pull https://git.openjdk.java.net/jdk pull/4135/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4135

View PR using the GUI difftool:
$ git pr show -t 4135

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4135.diff

GavinBierman · 2021-05-20T21:08:11Z

/csr

bridgekeeper · 2021-05-20T21:08:17Z

👋 Welcome back gbierman! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2021-05-20T21:10:57Z

@GavinBierman this pull request will not be integrated until the CSR request JDK-8265131 for issue JDK-8265130 has been approved.

openjdk · 2021-05-20T21:13:20Z

@GavinBierman The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2021-05-20T21:17:49Z

Webrevs

mlchung

Looks like constants.patch is accidentally added in this commit. That should be removed.

mlchung · 2021-05-20T21:32:08Z

src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java

@@ -56,7 +56,7 @@
 *
 * @since 12
 */
-public abstract class DynamicConstantDesc<T>
+non-sealed public abstract class DynamicConstantDesc<T>


can you explain why DynamicConstantDesc is non-sealed?

It's a permitted subclass of ConstantDesc, so it must be either final, sealed, or non-sealed. Making it non-sealed means it can still be extended.

I should have made it clear. I was expecting DynamicConstantDesc should be sealed. Do you expect non-JDK implementation class extending DynamicConstantDesc?

From the ConstantDesc Javadoc:

* <p>Non-platform classes should not implement {@linkplain ConstantDesc} directly. * Instead, they should extend {@link DynamicConstantDesc} (as {@link EnumDesc} * and {@link VarHandleDesc} do.)

Thanks. I missed this javadoc.

liach · 2021-05-21T03:26:49Z

src/java.base/share/classes/java/lang/constant/ClassDesc.java

 * @see ConstantDescs
 *
 * @since 12
 */
-public interface ClassDesc
+sealed public interface ClassDesc


Should move sealed behind public per the blessed modifier order from JLS. https://docs.oracle.com/javase/specs/jls/se16/preview/specs/sealed-classes-jls.html#jls-8.1.1

Per http://openjdk.java.net/jeps/409 the finalized sealed classes have no difference from that in 16, so the order suggested there should be valid.

Thanks. Now changed.

JornVernee

This looks good to me.

JornVernee · 2021-05-21T13:25:33Z

Does this require a CSR as well?

I see a CSR here for another change that seals a hierarchy: https://bugs.openjdk.java.net/browse/JDK-8267506

JornVernee · 2021-05-21T13:29:40Z

Ah, nvm, it's already indicated on the PR :)

vicente-romero-oracle

looks good to me

openjdk · 2021-06-01T17:13:36Z

@GavinBierman 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:

8265130: Make ConstantDesc class hierarchy sealed

Reviewed-by: mchung, jvernee, vromero

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 147 new commits pushed to the master branch:

c2c0208: 8267089: Use typedef KVHashtable for ID2KlassTable
ccfcd92: 8180571: Refactor sun/security/pkcs11 shell tests to plain java tests and fix failures
40e4171: 8264800: cleanup Threads_lock comments in JVM/TI function headers
b17b11e: 8267562: G1: Missing BOT in Open Archive regions causes long pauses
2b33835: 8265148: StackWatermarkSet being updated during AsyncGetCallTrace
64ba2f0: 8265431: Add -fno-delete-null-pointer-checks to clang builds
d4b4cae: 8268023: Improve diagnostic for HandshakeFailureTest
9d8ad2e: 8267990: Revisit some uses of synchronized in the HttpClient API
36dc268: 8268036: ProblemList gtest/AsyncLogGtest.java
74b70a5: 8036580: CHA: improve default method support
... and 137 more: https://git.openjdk.java.net/jdk/compare/aba22656829913d5f8d619a184c929a7de8431e4...master

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@mlchung, @JornVernee, @vicente-romero-oracle) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

GavinBierman · 2021-06-01T21:35:03Z

/integrate

openjdk · 2021-06-01T21:36:28Z

@GavinBierman
Your change (at version c36075d) is now ready to be sponsored by a Committer.

vicente-romero-oracle · 2021-06-02T00:48:31Z

/sponsor

openjdk · 2021-06-02T00:50:17Z

@vicente-romero-oracle @GavinBierman Since your change was applied there have been 151 commits pushed to the master branch:

00c7aee: 8267861: Update SourceVersion with new language features in 16 and 17
cb7128b: 8266950: Remove vestigial support for non-strict floating-point execution
8624cb5: 8266530: HotSpot changes for JEP 306
0ae4ceb: 8244146: javac changes for JEP 306
c2c0208: 8267089: Use typedef KVHashtable for ID2KlassTable
ccfcd92: 8180571: Refactor sun/security/pkcs11 shell tests to plain java tests and fix failures
40e4171: 8264800: cleanup Threads_lock comments in JVM/TI function headers
b17b11e: 8267562: G1: Missing BOT in Open Archive regions causes long pauses
2b33835: 8265148: StackWatermarkSet being updated during AsyncGetCallTrace
64ba2f0: 8265431: Add -fno-delete-null-pointer-checks to clang builds
... and 141 more: https://git.openjdk.java.net/jdk/compare/aba22656829913d5f8d619a184c929a7de8431e4...master

Your commit was automatically rebased without conflicts.

Pushed as commit 379376f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

mlbridge · 2021-06-02T13:00:04Z

Mailing list message from Brian Goetz on core-libs-dev:

The motivation here is that we wanted to preserve the ability to
describe "special" indy sites with special objects.? The standard
implementation can describe any indy site (bootstrap, static args,
invocation name and type), but some indy sites (e.g., lambda capture)
are "special".? It would be reasonable for a classfile parser to
"uplevel" such sites, so that (a) parsers could provide implementations
that interpret the semantics of the bootstrap argument list relative to
the known bootstrap method, and (b) so that class generators (including
compilers) could provide higher-level descriptions of the indy.? This
allows us to abstract away from the details of compiler translation, by
"unlowering" translation-by-indy for known bootstraps.

On 5/20/2021 7:43 PM, Mandy Chung wrote:

mlbridge · 2021-06-02T13:00:05Z

Mailing list message from Brian Goetz on core-libs-dev:

The motivation here is that we wanted to preserve the ability to
describe "special" indy sites with special objects.? The standard
implementation can describe any indy site (bootstrap, static args,
invocation name and type), but some indy sites (e.g., lambda capture)
are "special".? It would be reasonable for a classfile parser to
"uplevel" such sites, so that (a) parsers could provide implementations
that interpret the semantics of the bootstrap argument list relative to
the known bootstrap method, and (b) so that class generators (including
compilers) could provide higher-level descriptions of the indy.? This
allows us to abstract away from the details of compiler translation, by
"unlowering" translation-by-indy for known bootstraps.

On 5/20/2021 7:43 PM, Mandy Chung wrote:

8265130: Make ConstantDesc class hierarchy sealed

8084e90

openjdk bot added rfr Pull request is ready for review csr Pull request needs approved CSR before integration labels May 20, 2021

openjdk bot added the core-libs core-libs-dev@openjdk.org label May 20, 2021

Removing javadoc comments about future use of sealed

1cd5462

mlchung reviewed May 20, 2021

View reviewed changes

Removing file constants.patch added by mistake

c8f632f

liach reviewed May 21, 2021

View reviewed changes

Reordering class modifiers

c36075d

JornVernee approved these changes May 21, 2021

View reviewed changes

vicente-romero-oracle approved these changes May 21, 2021

View reviewed changes

mlchung approved these changes May 21, 2021

View reviewed changes

openjdk bot removed the csr Pull request needs approved CSR before integration label Jun 1, 2021

openjdk bot added the ready Pull request is ready to be integrated label Jun 1, 2021

openjdk bot added the sponsor Pull request is ready to be sponsored label Jun 1, 2021

openjdk bot closed this Jun 2, 2021

openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated labels Jun 2, 2021

openjdk bot removed the rfr Pull request is ready for review label Jun 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8265130: Make ConstantDesc class hierarchy sealed #4135

8265130: Make ConstantDesc class hierarchy sealed #4135

GavinBierman commented May 20, 2021 •

edited by openjdk bot

Loading

GavinBierman commented May 20, 2021

bridgekeeper bot commented May 20, 2021

openjdk bot commented May 20, 2021

openjdk bot commented May 20, 2021

mlbridge bot commented May 20, 2021 •

edited

Loading

mlchung left a comment •

edited

Loading

mlchung May 20, 2021

GavinBierman May 20, 2021 •

edited

Loading

mlchung May 20, 2021

Thihup May 20, 2021 •

edited by bridgekeeper bot

Loading

mlchung May 20, 2021

liach May 21, 2021

GavinBierman May 21, 2021

JornVernee left a comment

JornVernee commented May 21, 2021

JornVernee commented May 21, 2021

vicente-romero-oracle left a comment

openjdk bot commented Jun 1, 2021 •

edited

Loading

GavinBierman commented Jun 1, 2021

openjdk bot commented Jun 1, 2021

vicente-romero-oracle commented Jun 2, 2021

openjdk bot commented Jun 2, 2021

mlbridge bot commented Jun 2, 2021

mlbridge bot commented Jun 2, 2021

8265130: Make ConstantDesc class hierarchy sealed #4135

8265130: Make ConstantDesc class hierarchy sealed #4135

Conversation

GavinBierman commented May 20, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

GavinBierman commented May 20, 2021

bridgekeeper bot commented May 20, 2021

openjdk bot commented May 20, 2021

openjdk bot commented May 20, 2021

mlbridge bot commented May 20, 2021 • edited Loading

Webrevs

mlchung left a comment • edited Loading

Choose a reason for hiding this comment

mlchung May 20, 2021

Choose a reason for hiding this comment

GavinBierman May 20, 2021 • edited Loading

Choose a reason for hiding this comment

mlchung May 20, 2021

Choose a reason for hiding this comment

Thihup May 20, 2021 • edited by bridgekeeper bot Loading

Choose a reason for hiding this comment

mlchung May 20, 2021

Choose a reason for hiding this comment

liach May 21, 2021

Choose a reason for hiding this comment

GavinBierman May 21, 2021

Choose a reason for hiding this comment

JornVernee left a comment

Choose a reason for hiding this comment

JornVernee commented May 21, 2021

JornVernee commented May 21, 2021

vicente-romero-oracle left a comment

Choose a reason for hiding this comment

openjdk bot commented Jun 1, 2021 • edited Loading

GavinBierman commented Jun 1, 2021

openjdk bot commented Jun 1, 2021

vicente-romero-oracle commented Jun 2, 2021

openjdk bot commented Jun 2, 2021

mlbridge bot commented Jun 2, 2021

mlbridge bot commented Jun 2, 2021

GavinBierman commented May 20, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented May 20, 2021 •

edited

Loading

mlchung left a comment •

edited

Loading

GavinBierman May 20, 2021 •

edited

Loading

Thihup May 20, 2021 •

edited by bridgekeeper bot

Loading

openjdk bot commented Jun 1, 2021 •

edited

Loading