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

8267948: [lword] Core reflection and method handles support for L/Q model #436

Closed
wants to merge 1 commit into from

Conversation

mlchung
Copy link
Member

@mlchung mlchung commented Jun 3, 2021

This PR implements the primary (ref) mirror and secondary (val) mirror for primitive classes [1]. The following APIs returning Class return the primary mirror:

Object::getClass
Class::getClass
Class::forName
java.lang.reflect.Member::getDeclaringClass (i.e. Field, Method and Constructor)

Class::getName and Class::getSimpleName return the same name for the ref and val mirror of a primitive class. Therefore, Class.forName(type.getName()) will work if the type is either ref or val mirror.

Class::getTypeName returns Foo.ref if this Class is a primitive reference type. (we will revisit this for reference-favoring primitive class.)

New proposed APIs:
Class::isPrimitiveClass returns true for the ref and val mirror of a primitive class.

Class::isPrimaryType returns true if this Class object represents the primary mirror
Class::asPrimaryType returns the primary type of this class or interface.

  • An alternative I considered is Class::isReferenceType and Class::asReferenceType but they will have to specify the special case for primitive types which are not (yet) primitive classes. I want to go with this initial patch to make progress for the prototype. We will follow up the API discussion next.

Class::isValueType returns true if this Class object represents the primary mirror
Class::asValueType returns the primary type of this class or interface.

Several tests fail when running with -Xcomp which may depend on the second phase JIT support work (JDK-8267932) which depends on this work to make progress.

[1] https://github.com/openjdk/valhalla-docs/blob/main/site/design-notes/state-of-valhalla/03-vm-model.md


Progress

  • Change must not contain extraneous whitespace

Issue

  • JDK-8267948: [lword] Core reflection and method handles support for L/Q model

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 436

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 3, 2021

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

@openjdk
Copy link

openjdk bot commented Jun 3, 2021

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

8267948: [lword] Core reflection and method handles support for L/Q model

Reviewed-by: fparain, rriggs

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 no new commits pushed to the lqagain branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

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

@mlbridge
Copy link

mlbridge bot commented Jun 3, 2021

Webrevs

Copy link
Collaborator

@fparain fparain left a comment

Choose a reason for hiding this comment

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

The runtime part looks good to me.

Fred

Copy link
Collaborator

@RogerRiggs RogerRiggs left a comment

Choose a reason for hiding this comment

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

Generally looks ok.
I think there will be confusion between Class.isPrimitive() and Class.isPrimitiveClass()
but that's the way it is.

@mlchung
Copy link
Member Author

mlchung commented Jun 7, 2021

/integrate

@openjdk openjdk bot closed this Jun 7, 2021
@openjdk openjdk bot added integrated and removed ready labels Jun 7, 2021
@openjdk
Copy link

openjdk bot commented Jun 7, 2021

@mlchung Pushed as commit d9d96e8.

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

@openjdk openjdk bot removed the rfr label Jun 7, 2021
@mlchung
Copy link
Member Author

mlchung commented Jun 7, 2021

Generally looks ok.

Thanks Roger.

I think there will be confusion between Class.isPrimitive() and Class.isPrimitiveClass()
but that's the way it is.

It probably becomes not so much of an issue when JEP 402 is in.

@mlchung mlchung deleted the lq-reflection branch June 17, 2021 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants