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

8265356: need code example for getting canonical constructor of a Record #3556

Closed
wants to merge 5 commits into from

Conversation

@amaembo
Copy link
Contributor

@amaembo amaembo commented Apr 17, 2021

I decided to show a complete static method in the example, so it could be copied to user utility class as is. Not sure if it's reasonable to add assert cls.isRecord(); there. Also I don't know whether there's a limitation on max characters in the sample code. Probable a line break in static <T extends Record>\nConstructor<T> getCanonicalConstructor(Class<T> cls) is unnecessary.


Aside from this PR, I've found a couple of things to clean up in java.lang.Class:

  1. There's erroneous JavaDoc link in getSimpleName() JavaDoc (introduced by @jddarcy in #3038). It should be #isArray() instead of isArray().
  2. Methods Atomic::casAnnotationType and Atomic::casAnnotationData have unused type parameters <T>.
  3. Probably too much but AnnotationData can be nicely converted to a record! Not sure, probably nobody wants to have java.lang.Record initialized too early or increasing the footprint of such a basic class in the metaspace, so I don't insist on this.
    private record AnnotationData(
        Map<Class<? extends Annotation>, Annotation> annotations,
        Map<Class<? extends Annotation>, Annotation> declaredAnnotations,
        // Value of classRedefinedCount when we created this AnnotationData instance
        int redefinedCount) {
    }

Please tell me if it's ok to fix 1 and 2 along with this PR.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8265356: need code example for getting canonical constructor of a Record

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3556

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 17, 2021

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

@openjdk openjdk bot added the rfr label Apr 17, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 17, 2021

@amaembo The following label will be automatically applied to this pull request:

  • core-libs

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.

@openjdk openjdk bot added the core-libs label Apr 17, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 17, 2021

Webrevs

@stuart-marks
Copy link
Member

@stuart-marks stuart-marks commented Apr 20, 2021

Thanks for writing this example.

I think that the example lines can be longer. I'd suggest putting the main part of the method declaration on the same line as static <T extends Record>, but leaving the throws clause on the next line.

I think including the small cleanups (1) and (2) in this PR is fine. Changing AnnotationData to be a record seems like it might have other effects, so I'd leave that one out.

One other thing I'd like to see is a link to this example code from places where people are likely to look for it. The class doc for java.lang.Record has a definition for "canonical constructor" so it would be nice to link to the example here. Something like "For further information about how to find the canonical constructor reflectively, see Class::getRecordComponents." (With appropriate javadoc markup.) This could either be a parenthetical comment somewhere in the "canonical constructor" discussion, or possibly a separate paragraph in the @apiNote section below.

@openjdk openjdk bot removed the rfr label Apr 24, 2021
@openjdk openjdk bot added the rfr label Apr 24, 2021
@amaembo
Copy link
Contributor Author

@amaembo amaembo commented Apr 24, 2021

@stuart-marks thank you for review. How about this note in Record class? I wrote it in a more general manner, without mentioning canonical constructors explicitly. Is it enough?

Copy link
Member

@stuart-marks stuart-marks left a comment

Overall looks fine modulo adding @apiNote. Since the changes are all either editorial/markup or informational, I don't think this needs a CSR. Well, the removal of an unused type variable strictly constitutes a signature change, but those methods aren't public APIs so I still think it's ok without a CSR.

@@ -2361,6 +2361,18 @@ public Method getMethod(String name, Class<?>... parameterTypes)
* Conversely, if {@link #isRecord()} returns {@code true}, then this method
* returns a non-null value.
*

This comment has been minimized.

@stuart-marks

stuart-marks May 1, 2021
Member

I forgot to mention, this example should be within an @apiNote.

@openjdk
Copy link

@openjdk openjdk bot commented May 1, 2021

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

8265356: need code example for getting canonical constructor of a Record

Reviewed-by: smarks

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 265 new commits pushed to the master branch:

  • f86b70c: 8266328: C2: Remove InlineWarmCalls
  • 928d632: 8252237: C2: Call to compute_separating_interferences has wrong argument order
  • 50fa162: 8266389: ProblemList java/awt/Graphics2D/DrawString/DrawRotatedStringUsingRotatedFont.java on generic-all
  • dd05158: 8266155: Convert java.base to use Stream.toList()
  • c36c63a: 8260560: convert jdeps and jdeprscan tools to use Stream.toList()
  • 096e9e5: 8266318: Switch to macos prefix for macOS bundles
  • 0544a73: 8255227: java/net/httpclient/FlowAdapterPublisherTest.java intermittently failing with TestServer: start exception: java.io.IOException: Invalid preface
  • 48bb996: 8266220: keytool still prompt for store password on a password-less pkcs12 file if -storetype pkcs12 is specified
  • 87de5b7: 8266040: Lanai: Incorrect calculations of clipping boundaries
  • eb8db12: 8263396: Atomic::CmpxchgByteUsingInt::set_byte_in_int needs an explicit cast
  • ... and 255 more: https://git.openjdk.java.net/jdk/compare/66f89870f226f499ce8d89a8b51357484bf7f694...master

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 master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label May 1, 2021
@amaembo
Copy link
Contributor Author

@amaembo amaembo commented May 1, 2021

Thank you! I also changed 'This method' to 'The following method', as it looks like this wording is more commonly used in OpenJDK specs.

@amaembo
Copy link
Contributor Author

@amaembo amaembo commented May 1, 2021

/integrate

@openjdk openjdk bot closed this May 1, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels May 1, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 1, 2021

@amaembo Since your change was applied there have been 265 commits pushed to the master branch:

  • f86b70c: 8266328: C2: Remove InlineWarmCalls
  • 928d632: 8252237: C2: Call to compute_separating_interferences has wrong argument order
  • 50fa162: 8266389: ProblemList java/awt/Graphics2D/DrawString/DrawRotatedStringUsingRotatedFont.java on generic-all
  • dd05158: 8266155: Convert java.base to use Stream.toList()
  • c36c63a: 8260560: convert jdeps and jdeprscan tools to use Stream.toList()
  • 096e9e5: 8266318: Switch to macos prefix for macOS bundles
  • 0544a73: 8255227: java/net/httpclient/FlowAdapterPublisherTest.java intermittently failing with TestServer: start exception: java.io.IOException: Invalid preface
  • 48bb996: 8266220: keytool still prompt for store password on a password-less pkcs12 file if -storetype pkcs12 is specified
  • 87de5b7: 8266040: Lanai: Incorrect calculations of clipping boundaries
  • eb8db12: 8263396: Atomic::CmpxchgByteUsingInt::set_byte_in_int needs an explicit cast
  • ... and 255 more: https://git.openjdk.java.net/jdk/compare/66f89870f226f499ce8d89a8b51357484bf7f694...master

Your commit was automatically rebased without conflicts.

Pushed as commit 3e667cc.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants