-
Notifications
You must be signed in to change notification settings - Fork 5.9k
8356632: Fix remaining {@link/@linkplain} tags with refer to private/protected types in java.base #25287
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
8356632: Fix remaining {@link/@linkplain} tags with refer to private/protected types in java.base #25287
Conversation
👋 Welcome back nbenalla! A progress list of the required criteria for merging this PR into |
@nizarbenalla 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 113 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 |
@nizarbenalla The following labels 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 lists. If you would like to change these labels, use the /label pull request command. |
@@ -4890,7 +4890,7 @@ private static MethodHandle identityOrVoid(Class<?> type) { | |||
* @param type the type of the desired method handle | |||
* @return a constant method handle of the given type, which returns a default value of the given return type | |||
* @throws NullPointerException if the argument is null | |||
* @see MethodHandles#primitiveZero | |||
* @see <code>primitiveZero(Wrapper)</code> |
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.
Just #zero(Class)
should be sufficient. This is probably changed by intellij during my original work to consolidate zero and constant.
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.
From #23706 (comment):
* @see <code>primitiveZero(Wrapper)</code> | |
* @see MethodHandles#zero |
* @see #readResolve | ||
* @see #writeObject | ||
* @see <code>MethodType.readResolve()</code> | ||
* @see <code>MethodType.writeObject(ObjectOutputStream s)</code> |
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.
This is already not generated by the javadoc as this method is private. Why is this check against a private method?
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.
They are documented in this page
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.
Okay -- if there is some link to the private serial-related methods even in a public javadoc run, I think it would be preferable if links to those methods could be resolved by javadoc. Otherwise, if the links to private methods from a private method, would be valid in a private javadoc run, I thinks more refined version of warning would let those be.
As it is, if the see tags are going to stay, I don't think <code>foo</code>
markup should be used.
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 will revert the changes to MethoType.java
and try to make the links to the private methods resolvable. (In a future patch)
Webrevs
|
* returns a {@link CancellationException}); and | ||
* {@link #isCompletedAbnormally} is true if a task was either | ||
* cancelled or encountered an exception, in which case {@link | ||
* #getException} will return either the encountered exception or | ||
* #getException()} will return either the encountered exception or |
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 right too, the private overload of getException was added in JDK 22.
@@ -2020,7 +2020,7 @@ public Builder uses(String service) { | |||
|
|||
/** | |||
* Provides a service with one or more implementations. The package for | |||
* each {@link Provides#providers provider} (or provider factory) is | |||
* each {@link Provides#providers() provider} (or provider factory) is |
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.
okay
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.
Changes to java.net.Socket
LGTM
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.
Changes to MethodHandles look good. Please wait for a review from security developers for KEM.java.
@@ -708,7 +708,7 @@ public Encapsulator newEncapsulator(PublicKey publicKey, SecureRandom secureRand | |||
* If any extra information inside this object needs to be transmitted along | |||
* with the key encapsulation message so that the receiver is able to create | |||
* a matching decapsulator, it will be included as a byte array in the | |||
* {@link Encapsulated#params} field inside the encapsulation output. | |||
* {@link Encapsulated#params()} field inside the encapsulation output. |
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.
Good catch. The text must have been added before we changed Encapsulated
from a record to a normal class. Now that params
is a method instead of a record field, please consider rewriting the sentence to something like
it will be included as a byte array returned by the
{@link Encapsulated#params()} method within the
encapsulation output.
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.
The KEM.java
change looks good to me. Thanks.
All files have been reviewed. Thanks All! /integrate |
Going to push as commit bd09589.
Your commit was automatically rebased without conflicts. |
@nizarbenalla Pushed as commit bd09589. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Please review this patch to fix some
javadoc
bugs injava.base
.Certain
@link
tags used to refer to private fields instead of public APIs.A couple of
@see
tags in the serialization page referred to private methods, I updated the javadoc in a way to not change the way it is displayed to users but also remove@link
tags to non-included types.TIA
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25287/head:pull/25287
$ git checkout pull/25287
Update a local copy of the PR:
$ git checkout pull/25287
$ git pull https://git.openjdk.org/jdk.git pull/25287/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 25287
View PR using the GUI difftool:
$ git pr show -t 25287
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25287.diff
Using Webrev
Link to Webrev Comment