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
8303820: Simplify type metadata #12924
Conversation
getMetadata now returns Optional to allow for more functional style cloneWithMetadata is now protected
👋 Welcome back mcimadamore! A progress list of the required criteria for merging this PR into |
@mcimadamore The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
@@ -328,7 +323,7 @@ public <Z> Type map(TypeMapping<Z> mapping) { | |||
* and with given constant value | |||
*/ | |||
public Type constType(Object constValue) { |
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.
so now any type can have a constant value? shouldn't this be restricted to primitives and Strings?
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.
Yeah - not sure which way to go - in reality we have a also a couple of types which implement this method and just return this
. Only the toplevel Type
declaration throws. I'm happy to revert to the old ways if that is preferred - it was mostly a way not to duplicate the method body twice (but that's not a big deal).
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 would keep the original semantics and we can relax it later if needed, but I don't have a strong preference tbh
nice patch, nice simplification, just added a question |
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.
looks good thanks
@mcimadamore 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 20 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 b9951dd.
Your commit was automatically rebased without conflicts. |
@mcimadamore Pushed as commit b9951dd. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
This patch simplifies the TypeMetadata API. TypeMetadata can be used as a side-channel, to attach extra information to a javac type (type annotations, constant values).
While TypeMetadata provides the right knobs to compare types (see
Type::equalsIgnoreMetadata
), which is used uniformly across the type-system related routines (e.g. subtyping, type equality and type containment), the TypeMetadata API is also rather cumbersome to use.The only way to add a new metadata to a type is to "combine" the metadata into an existing one, which feels odd, and probably too biased by the type annotations requirements.
This patch simplifies this: now TypeMetadata is a simple marker interface. There can be many implementations - one is
Annotations
(used to store type annotations), another isConstantValue
(used to store a type's constant value).A type is then associated with a
List<TypeMetadata>
, and there are methods to add/drop/get metadata. These methods are used to provide support for annotated and constant types.The resulting code feels simpler: to add a new metadata, one only has to define a new class/record inside TypeMetadata, and that's pretty much it. Javac will truck the metadata around in the correct way (as it did before).
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/12924/head:pull/12924
$ git checkout pull/12924
Update a local copy of the PR:
$ git checkout pull/12924
$ git pull https://git.openjdk.org/jdk pull/12924/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 12924
View PR using the GUI difftool:
$ git pr show -t 12924
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12924.diff