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

8273018: [lworld] Property annotation propagation to <init> lacks in primitive records #541

Closed

Conversation

jespersm
Copy link
Contributor

@jespersm jespersm commented Aug 31, 2021

This fix copies parameter names and annotations from constructor into primitive class factory.


Progress

  • Change must not contain extraneous whitespace

Issues

  • JDK-8273018: [lworld] Property annotation propagation to lacks in primitive records
  • JDK-8273202: [lworld] MethodParameters are not generated for factories for primitive records

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 541

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 31, 2021

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

@openjdk openjdk bot added the rfr label Aug 31, 2021
@jespersm
Copy link
Contributor Author

jespersm commented Aug 31, 2021

/integrate

@mlbridge
Copy link

mlbridge bot commented Aug 31, 2021

Webrevs

@openjdk
Copy link

openjdk bot commented Aug 31, 2021

@jespersm This PR has not yet been marked as ready for integration.

@sadayapalam
Copy link
Collaborator

sadayapalam commented Sep 1, 2021

The title of the PR seems to be incorrect ??

@jespersm jespersm changed the title 8273018: Emit parameter annotations on primitive record factories 8273018: [lworld] Property annotation propagation to <init> lacks in primitive records Sep 1, 2021
@openjdk
Copy link

openjdk bot commented Sep 1, 2021

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

8273018: [lworld] Property annotation propagation to <init> lacks in primitive records
8273202: [lworld] MethodParameters are not generated for factories for primitive records

Reviewed-by: sadayapalam

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 9 new commits pushed to the lworld branch:

  • a5cf519: Adjust problem list for initial aarch64 testing
  • c64b160: Fix header format for build setup
  • 8bed69a: 8273650: [lworld] Interpreter fails with fatal error: DEBUG MESSAGE: klass not initialized
  • e075eaf: 8270477: [lworld] bytecode testing API does not emit Q type descriptors
  • 0c0c9e1: 8267697: [lworld] [lw3] VM crashes during heap dump if Java heap contains flat arrays
  • 3111124: 8273554: [lworld] Runtime tests should not explicitly set -Xint/-Xcomp
  • 252b04b: 8273323: [lworld] Fix post-parse call devirtualization with inline type receiver
  • 59fc2db: 8267665: [lworld] Scalarization of nullable inline types
  • 45b6363: [lworld] Removed TestNativeClone.java from ProblemList now that 8270098 is fixed

Please see this link for an up-to-date comparison between the source branch of this pull request and the lworld branch.
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 (@sadayapalam) 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).

@openjdk openjdk bot added the ready label Sep 1, 2021
@jespersm
Copy link
Contributor Author

jespersm commented Sep 1, 2021

Fixed name of PR. The name of the issue is slightly off, I guess it should be ‘parameter annotation’, really.

- Keep type parameters
- Provide useful test for primitive records in "RecordReading"
@jespersm
Copy link
Contributor Author

jespersm commented Sep 7, 2021

I combined this fix (which was incomplete, since it "de-erased" the method types incorrectly) with the fix for the problem in JDK-8273202, and a more useful example of incorrect compilation of primitive records.

I don't like hacking the erasure_field of the MethodSymbol, but I don't see another option, since the other lowering passes do similar nasty tricks when recording outer instances and capturing locals for inner classes.

@sadayapalam
Copy link
Collaborator

sadayapalam commented Sep 15, 2021

If you want to close more than issue with the same PR, I think you need to add the other issue here with /issue

Also please mention what tests have been run. Thanks

@sadayapalam
Copy link
Collaborator

sadayapalam commented Sep 20, 2021

OK, Thanks for the clarifications. If the suggested changes are made, we can take this forward.

@jespersm
Copy link
Contributor Author

jespersm commented Sep 22, 2021

/issue 8273202

@openjdk
Copy link

openjdk bot commented Sep 22, 2021

@jespersm
Adding additional issue to issue list: 8273202: [lworld] MethodParameters are not generated for factories for primitive records.

@jespersm
Copy link
Contributor Author

jespersm commented Sep 22, 2021

@sadayapalam: I've now incorporated the requested review changes.

I've run the tier1 tests, all is green here.

Copy link
Collaborator

@sadayapalam sadayapalam left a comment

Looks good, please proceed to integrate, Thanks so much Jesper

@jespersm
Copy link
Contributor Author

jespersm commented Sep 23, 2021

/integrate

@openjdk openjdk bot added the sponsor label Sep 23, 2021
@openjdk
Copy link

openjdk bot commented Sep 23, 2021

@jespersm
Your change (at version 4c5ef8e) is now ready to be sponsored by a Committer.

@sadayapalam
Copy link
Collaborator

sadayapalam commented Sep 23, 2021

/sponsor

@openjdk
Copy link

openjdk bot commented Sep 23, 2021

Going to push as commit 2688bf6.
Since your change was applied there have been 9 commits pushed to the lworld branch:

  • a5cf519: Adjust problem list for initial aarch64 testing
  • c64b160: Fix header format for build setup
  • 8bed69a: 8273650: [lworld] Interpreter fails with fatal error: DEBUG MESSAGE: klass not initialized
  • e075eaf: 8270477: [lworld] bytecode testing API does not emit Q type descriptors
  • 0c0c9e1: 8267697: [lworld] [lw3] VM crashes during heap dump if Java heap contains flat arrays
  • 3111124: 8273554: [lworld] Runtime tests should not explicitly set -Xint/-Xcomp
  • 252b04b: 8273323: [lworld] Fix post-parse call devirtualization with inline type receiver
  • 59fc2db: 8267665: [lworld] Scalarization of nullable inline types
  • 45b6363: [lworld] Removed TestNativeClone.java from ProblemList now that 8270098 is fixed

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Sep 23, 2021
@openjdk openjdk bot added the integrated label Sep 23, 2021
@openjdk
Copy link

openjdk bot commented Sep 23, 2021

@sadayapalam @jespersm Pushed as commit 2688bf6.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants