Skip to content
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

8242612: [lworld] Javac should not expressly encode the new super interface types in class files #50

Closed
wants to merge 4 commits into from

Conversation

sadayapalam
Copy link
Collaborator

@sadayapalam sadayapalam commented May 18, 2020

Jim, Could you please review this change ? TIA.

Changes:

- Withdraw the new top type InlineObject (per design)
- Do not encode the new top types in class files as this is noisy and
  very disruptive and let the static compiler and the dynamic environments
  inject these types as needed. (since classes compiled with legacy compilers
  will not encode these top types anyway the VM has to resort to injection,
  why not resort to injection for everything is the motivation)
- Adjust some code that is surprised by super interfaces being empty.

More background is available in https://bugs.openjdk.java.net/browse/JDK-8242612


Progress

  • Change must not contain extraneous whitespace

Issues

  • JDK-8242612: [lworld] Javac should not expressly encode the new super interface types in class files ⚠️ Title mismatch between PR and JBS.
  • JDK-8237954: [lworld] @ignored tests need to be tweaked and reenabled.

Reviewers

  • Jim Laskey (jlaskey - no project role) ⚠️ Review applies to ecb41ea

Download

$ git fetch https://git.openjdk.java.net/valhalla pull/50/head:pull/50
$ git checkout pull/50

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 18, 2020

👋 Welcome back sadayapalam! A progress list of the required criteria for merging this PR into lworld will be added to the body of your pull request.

@openjdk
Copy link

@openjdk openjdk bot commented May 18, 2020

@sadayapalam This change now passes all automated pre-integration checks, type /integrate in a new comment to proceed. After integration, the commit message will be:

8242612: lworld] Javac should not expressly encode the new super interface types in class files
8237954: [lworld] @ignored tests need to be tweaked and reenabled.

Reviewed-by: jlaskey
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there has been 1 commit pushed to the lworld branch:

  • 9a5e8f3: 8242612: lworld] Javac should not expressly encode the new super interface types in class files

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge lworld into your branch, and then specify the current head hash when integrating, like this: /integrate 9a5e8f3921ae281fd64fddefdd3417ab3b90406f.

➡️ To integrate this PR with the above commit message to the lworld branch, type /integrate in a new comment.

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 18, 2020

Webrevs

Type it = types.makeIntersectionType(((ClassType)type).interfaces_field, true);
ClassType ct = (ClassType) type;
Type it;
if (ct.interfaces_field == null || ct.interfaces_field.isEmpty()) {
Copy link
Member

@JimLaskey JimLaskey May 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming that you are using this to indicate no top interfaces. Is this safe (non-hackish)?

Copy link
Collaborator Author

@sadayapalam sadayapalam May 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is safe. Basically earlier on all value classes had at least one interface (InlineObject) and possibly more. Now the magic top type InlineObject going away, values may have no super interfaces at all and this would upset makeIntersectionType which first asserts that it has not been called with an empty list of types.

Type wcb;
if (qualifierType.isValue()) {
ClassType ct = (ClassType) qualifierType;
if (ct.interfaces_field == null || ct.interfaces_field.isEmpty()) {
Copy link
Member

@JimLaskey JimLaskey May 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same - maybe define a predicate for this.

@sadayapalam
Copy link
Collaborator Author

@sadayapalam sadayapalam commented May 18, 2020

/solves JDK-8237954

@sadayapalam
Copy link
Collaborator Author

@sadayapalam sadayapalam commented May 18, 2020

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented May 18, 2020

@sadayapalam
Adding additional issue to solves list: 8237954: [lworld] @ignored tests need to be tweaked and reenabled..

@openjdk openjdk bot closed this May 18, 2020
@openjdk openjdk bot added integrated and removed ready labels May 18, 2020
@openjdk
Copy link

@openjdk openjdk bot commented May 18, 2020

@sadayapalam
Pushed as commit 9a5e8f3.

@openjdk openjdk bot removed the rfr label May 18, 2020
@sadayapalam sadayapalam deleted the JDK-8242612 branch May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants