-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
JDK-8281766: Update java.lang.reflect.Parameter to implement Member #7468
Conversation
👋 Welcome back darcy! A progress list of the required criteria for merging this PR into |
/csr |
@jddarcy this pull request will not be integrated until the CSR request JDK-8281767 for issue JDK-8281766 has been approved. |
Mailing list message from David Holmes on core-libs-dev: Hi Joe, On 15/02/2022 11:22 am, Joe Darcy wrote:
But a parameter is not a Member! David |
@@ -27,6 +27,7 @@ | |||
import java.lang.annotation.*; | |||
import java.util.HashMap; | |||
import java.util.Map; | |||
import java.util.Set; |
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.
java.util.Set
doesn’t seem to be referenced from anywhere else in this class.
I also consider that Parameter is not a Member. A member is a field, method, constructor. |
I agree with Mandy and David, a Member is the thingy inside a class, so a parameter is no a member, it has no declaring class. |
Technically a constructor is not a Member either, but the reflection API can get away with lumping constructors in with methods. The JLS definition of Member comes from 8.1.6 and 8.2 - the key definition being in 8.1.6: "A class body may contain declarations of members of the class, that is, fields (§8.3), |
Hmmm. Okay; I'll concede to the consensus here and withdraw the PR, CSR, etc. Parameter does mostly pass the "duck test" for me to be a Member, but I don't think it is essential for it to implement the interface (and also not essential to create one or more new interfaces separate from JLS terminology to characterize parameters and members/constructors as a more precise unified type than AnnotatedElement). Thanks. |
Retrofitting of Parameter to implement Member, an interface already implemented by similar reflective modeling classes.
Since Parameter is final, there is little compatibility impact from adding a new method.
Please also review the corresponding CSR:
https://bugs.openjdk.java.net/browse/JDK-8281767
I'll update the copyright year before pushing; I judged this as trivial enough to not require a regression test.
Progress
Issues
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7468/head:pull/7468
$ git checkout pull/7468
Update a local copy of the PR:
$ git checkout pull/7468
$ git pull https://git.openjdk.java.net/jdk pull/7468/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 7468
View PR using the GUI difftool:
$ git pr show -t 7468
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7468.diff