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
8278863: Add method ClassDesc::ofInternalName #9201
Conversation
👋 Welcome back asotona! A progress list of the required criteria for merging this PR into |
/csr needed |
@asotona this pull request will not be integrated until the CSR request JDK-8288670 for issue JDK-8278863 has been approved. |
Webrevs
|
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.
I've worked with ClassDesc quite a bit as well, and often found myself wanting to do the opposite. i.e. if I have a ClassDesc
, convert it to an internal name string (to give to ASM).
There are also methods to get the descriptor, as well as display name & package name from a ClassDesc
. So, I'd like to suggest adding a method to get the internal name from a ClassDesc
as well.
Co-authored-by: Jorn Vernee <JornVernee@users.noreply.github.com>
My initial thoughts were the same. Passing internal class name through the ClassDesc in and out is very frequent when using ASM library or similar. While constructing ClassDesc from various other forms (internal names, binary names, derivation from other ClassDesc, getting it from Class, etc...) is mostly user responsibility. For ASM case I would rather recommend a util class directly converting ClassDesc to ASM Type, while passing ClassDesc through String internal name is risky and not covering all cases. |
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.
Thanks for explaining.
@asotona 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! |
ping to keep the PR open |
@asotona 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 147 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 |
/integrate |
Going to push as commit 0fa7d9e.
Your commit was automatically rebased without conflicts. |
The symbolic constants API (
java.lang.constant
) was designed, in part, to provide bytecode parsing and generation APIs with a validated, common, symbolic form for constants in Java class files.There is an easy way to create a
ClassDesc
from a binary name (ClassDesc::of
) or a field descriptor (ClassDesc::ofDescriptor
) but not from an internal name. But, the internal name is common in low-level bytecode-manipulation code.This patch adds
ClassDesc::ofInternalName
static factory method that creates aClassDesc
from class internal name.Class internal name validation and extended ClassDescTest are also parts of this patch.
CSR is linked with the issue.
Please review.
Thank you,
Adam
Progress
Issues
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9201/head:pull/9201
$ git checkout pull/9201
Update a local copy of the PR:
$ git checkout pull/9201
$ git pull https://git.openjdk.org/jdk pull/9201/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 9201
View PR using the GUI difftool:
$ git pr show -t 9201
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9201.diff