-
Notifications
You must be signed in to change notification settings - Fork 6.3k
8333377: Migrate Generic Signature parsing to ClassFile API #19281
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
base: master
Are you sure you want to change the base?
Conversation
👋 Welcome back liach! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
|
I think I'll need to be one of the reviews on this, but don't anticipate being able to review it before JDK 23 rampdown. Therefore, I think this would be appropriate to be targeted to JDK 24. |
Indeed, I put the JBS issue to target release 24. I believe given the potential behavioral changes, rushing into release 23 is a risky choice. (the 4 new tests can target 23 in a separate patch, but they require removal of a few redundant assertions in old generic code) |
@liach This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
Keep-alive. |
@liach This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
@liach This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the |
/open This is becoming more necessary as valhalla brings changes to signature format. |
@liach This pull request is now open |
@liach This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
3 similar comments
@liach This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
@liach This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
@liach This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
@liach this pull request can not be integrated into git checkout feature/new-generic-info
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
@liach This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the |
86b89e5
to
dea7a23
Compare
@liach Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
Sorry for the force push - Too many commits meaning fixing merge conflicts in every single one of them was a bit complicated. |
@liach This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
Keep alive. One day there will be a reviewer... |
@liach This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
After a revisit to this PR, I think I might split the work of improving BytecodeDescriptor error message and tests covering failure-case behaviors into a prerequisite PR to this one, to ensure this PR preserves the failure behavior present on mainline. |
@liach This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
…new-generic-info
@liach This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the |
/open |
@liach This pull request is now open |
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.
Given that this basically rewrites most of sun.reflect.generics.*
, I feel like it might be better to just move these new files to jdk.internal.reflect.generics.*
.
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.
We should, in fact, remove as much legacy sun
packages as possible - for the particular package name choice, I think jdk.internal.reflect.type
might be better, as we are really reflecting on language types. But I think that effort will get its own ticket.
@liach This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
@liach This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the |
/open |
@liach This pull request is now open |
@liach This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
Core reflection's generic signature parsing uses an ancient library with outdated visitor pattern on a tree model and contains unnecessary boilerplates. This is a duplication of ClassFile API's signature model. We should just move to ClassFile API, which is more throughoutly tested as well.
To ensure compatibility, new tests are added to ensure consistent behavior when encountering malformed signatures or signatures with missing types. The reflective objects have been preserved and the only change is that lazy expansion now happens from CF objects, to reduce compatibility risks.
Progress
Issue
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19281/head:pull/19281
$ git checkout pull/19281
Update a local copy of the PR:
$ git checkout pull/19281
$ git pull https://git.openjdk.org/jdk.git pull/19281/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 19281
View PR using the GUI difftool:
$ git pr show -t 19281
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19281.diff
Using Webrev
Link to Webrev Comment