8252830: Correct missing javadoc comments in java.rmi module #79

RogerRiggs · 2020-09-08T15:19:40Z

Progress

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

Issue

JDK-8252830: Correct missing javadoc comments in java.rmi module

Reviewers

Daniel Fuchs (@dfuch - Reviewer)
Lance Andersen (@LanceAndersen - Reviewer)
Stuart Marks (@stuart-marks - Reviewer)
Chris Hegarty (@ChrisHegarty - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/79/head:pull/79
$ git checkout pull/79

bridgekeeper · 2020-09-08T15:21:46Z

👋 Welcome back rriggs! 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.

RogerRiggs · 2020-09-08T15:22:13Z

Add the missing comments where required by javadoc -Xdoclint to document public and serializable classes, methods, and fields.

openjdk · 2020-09-08T15:22:40Z

@RogerRiggs 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 (add|remove) "label" command.

RogerRiggs · 2020-09-08T15:27:02Z

/csr

openjdk · 2020-09-08T15:27:28Z

@RogerRiggs has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@RogerRiggs please create a CSR request and add link to it in JDK-8252830. This pull request cannot be integrated until the CSR request is approved.

mlbridge · 2020-09-08T15:30:05Z

Webrevs

dfuch · 2020-09-08T16:24:51Z

src/java.rmi/share/classes/java/rmi/activation/ActivationGroupDesc.java

+         * @param  in the {@code ObjectInputStream} from which data is read
+         * @throws IOException if an I/O error occurs
+         * @throws ClassNotFoundException if a serialized class cannot be loaded
+         *
         */
        private void readObject(ObjectInputStream in)


Hi Roger, you could take this opportunity to add the @Serial annotation to this method (here and in the other files you modified as well.

dfuch · 2020-09-08T16:25:14Z

src/java.rmi/share/classes/java/rmi/activation/ActivationID.java

@@ -228,6 +228,9 @@ public boolean equals(Object obj) {
     * java.rmi.server.RemoteObject RemoteObject}
     * <code>writeObject</code> method <b>serialData</b>
     * specification.
+     *
+     * @param  out the {@code ObjectOutputStream} to which data is written
+     * @throws IOException if an I/O error occurs
     **/
    private void writeObject(ObjectOutputStream out)


Same comment as above...

LanceAndersen

Hi Roger,

The combined changes to both commits look good

openjdk · 2020-09-08T19:02:38Z

@RogerRiggs This change now passes all automated pre-integration checks. In addition to the automated checks, the change must also fulfill all project specific requirements

After integration, the commit message will be:

8252830: Correct missing javadoc comments in java.rmi module

Reviewed-by: dfuchs, lancea, smarks, chegar

If you would like to add a summary, use the /summary command.
To credit additional contributors, use the /contributor command.
To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 44 commits pushed to the master branch:

41d29b7: 8252774: remove jdk.test.lib.FileInstaller action from graalunit tests
5b30a83: 8252778: remove jdk.test.lib.FileInstaller action from compiler/c2/stemmer test
8db3335: 8247928: Refactor G1ConcurrentMarkThread for mark abort
7ccf435: 8252846: Fix ppc/s390 after "8231441: AArch64: Initial SVE backend su…
d236cf4: 8252995: Non-PCH builds broken by JDK-8250961
6a00534: 8250961: Move Universe::update_heap_info_at_gc to CollectedHeap
f78f780: 8252889: Obsolete -XX:+InsertMemBarAfterArraycopy
f933961: 8252980: comment only changes extracted from JDK-8247281
4333942: 8250217: com.sun.tools.javac.api.JavacTaskImpl swallows compiler exceptions potentially producing false positive test results
5166094: 8252957: Wrong comment in CgroupV1Subsystem::cpu_quota
... and 34 more: https://git.openjdk.java.net/jdk/compare/5d2e79e2c38ff79c1d5712aa77d5ac98eef9f97c...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate 41d29b752b3b5835399bc5f119e4ba3aedc1d046.

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

dfuch

Otherwise looks good.

dfuch · 2020-09-08T19:36:41Z

src/java.rmi/share/classes/java/rmi/activation/ActivationID.java

@@ -228,6 +229,9 @@ public boolean equals(Object obj) {
     * java.rmi.server.RemoteObject RemoteObject}
     * <code>writeObject</code> method <b>serialData</b>
     * specification.
+     *
+     * @param  out the {@code ObjectOutputStream} to which data is written
+     * @throws IOException if an I/O error occurs
     **/
    private void writeObject(ObjectOutputStream out)


one @Serial still missing here?

stuart-marks · 2020-09-08T21:38:42Z

src/java.rmi/share/classes/java/rmi/server/RemoteObject.java

     */
+    @java.io.Serial
    private void writeObject(java.io.ObjectOutputStream out)
        throws java.io.IOException


It's a little odd to see an import of java.io.IOException (which is used by the @throws tag) but not to have it used here. There's another occurrence down below. These files have a mixture of imports and fully qualified names, which I find irritating, which might be nice to clean up. But you might find beyond the scope of this change. So, clean up as much of this as you like, or none if you prefer, no big deal.

RogerRiggs · 2020-09-09T13:16:31Z

Hi Stuart, I'm going to leave this for later, its a bit out of scope and every change prompts the people who have already reviewed it to need to review it again. Thanks, Roger

…

On 9/8/20 5:38 PM, Stuart Marks wrote: ***@***.**** commented on this pull request. ------------------------------------------------------------------------ In src/java.rmi/share/classes/java/rmi/server/RemoteObject.java <https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/79*discussion_r485210638__;Iw!!GqivPVa7Brio!OBTG2nlJMZowBn3gf_R72ZdA_MzJASDpLUuJFDC4fTAxUL_AnJ9Pfvctp470Fa4a$>: > */ + @java.io.Serial private void writeObject(java.io.ObjectOutputStream out) throws java.io.IOException It's a little odd to see an import of java.io.IOException (which is used by the ***@***.***| tag) but not to have it used here. There's another occurrence down below. These files have a mixture of imports and fully qualified names, which I find irritating, which might be nice to clean up. But you might find beyond the scope of this change. So, clean up as much of this as you like, or none if you prefer, no big deal. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/79*pullrequestreview-484510521__;Iw!!GqivPVa7Brio!OBTG2nlJMZowBn3gf_R72ZdA_MzJASDpLUuJFDC4fTAxUL_AnJ9Pfvctp0HjkJMk$>, or unsubscribe <https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AAIRSVBO6NWK7WC75GOJRETSE2P67ANCNFSM4RAD5TXQ__;!!GqivPVa7Brio!OBTG2nlJMZowBn3gf_R72ZdA_MzJASDpLUuJFDC4fTAxUL_AnJ9Pfvctp5LhysgN$>.

RogerRiggs · 2020-09-10T14:36:52Z

/integrate

openjdk · 2020-09-10T14:38:14Z

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

41d29b7: 8252774: remove jdk.test.lib.FileInstaller action from graalunit tests
5b30a83: 8252778: remove jdk.test.lib.FileInstaller action from compiler/c2/stemmer test
8db3335: 8247928: Refactor G1ConcurrentMarkThread for mark abort
7ccf435: 8252846: Fix ppc/s390 after "8231441: AArch64: Initial SVE backend su…
d236cf4: 8252995: Non-PCH builds broken by JDK-8250961
6a00534: 8250961: Move Universe::update_heap_info_at_gc to CollectedHeap
f78f780: 8252889: Obsolete -XX:+InsertMemBarAfterArraycopy
f933961: 8252980: comment only changes extracted from JDK-8247281
4333942: 8250217: com.sun.tools.javac.api.JavacTaskImpl swallows compiler exceptions potentially producing false positive test results
5166094: 8252957: Wrong comment in CgroupV1Subsystem::cpu_quota
... and 34 more: https://git.openjdk.java.net/jdk/compare/5d2e79e2c38ff79c1d5712aa77d5ac98eef9f97c...master

Your commit was automatically rebased without conflicts.

Pushed as commit 418e4a2.

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

openjdk#79) This patch update return_phi of AllocationState in Parse::do_exits(). (6107 Phi) = (6103 region, 3789 CheckCastPP, 3789 CheckCastPP) will become (3789 CheckCastPP) after first _gvn.transform(). If we still use 6107 Phi, we will have problem in process_phi later. Co-authored-by: Xin Liu <xxinliu@amazon.com>

Added missing comments in java.rmi

448061c

RogerRiggs closed this Sep 8, 2020

RogerRiggs reopened this Sep 8, 2020

openjdk bot added the core-libs core-libs-dev@openjdk.org label Sep 8, 2020

RogerRiggs changed the title ~~Added missing comments in java.rmi~~ 8252830: Added missing javadoc comments in java.rmi Sep 8, 2020

openjdk bot added the rfr Pull request is ready for review label Sep 8, 2020

openjdk bot added the csr Pull request needs approved CSR before integration label Sep 8, 2020

RogerRiggs changed the title ~~8252830: Added missing javadoc comments in java.rmi~~ 8252830: Correct missing javadoc comments in java.rmi Sep 8, 2020

RogerRiggs changed the title ~~8252830: Correct missing javadoc comments in java.rmi~~ 8252830: Correct missing javadoc comments in java.rmi module Sep 8, 2020

dfuch suggested changes Sep 8, 2020

View reviewed changes

Added @java.io.Serial annotation to serializable methods and fields

8aeaf5e

LanceAndersen approved these changes Sep 8, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 8, 2020

dfuch approved these changes Sep 8, 2020

View reviewed changes

Added java.io.Serial to java.rmi.activation.ActivationID

c5813e9

LanceAndersen approved these changes Sep 8, 2020

View reviewed changes

dfuch approved these changes Sep 8, 2020

View reviewed changes

stuart-marks reviewed Sep 8, 2020

View reviewed changes

stuart-marks approved these changes Sep 8, 2020

View reviewed changes

ChrisHegarty approved these changes Sep 9, 2020

View reviewed changes

openjdk bot removed the csr Pull request needs approved CSR before integration label Sep 9, 2020

openjdk bot closed this Sep 10, 2020

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 10, 2020

RogerRiggs deleted the 8252830-java.rmi-missing-comment branch September 10, 2020 15:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8252830: Correct missing javadoc comments in java.rmi module #79

8252830: Correct missing javadoc comments in java.rmi module #79

RogerRiggs commented Sep 8, 2020 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 8, 2020

RogerRiggs commented Sep 8, 2020

openjdk bot commented Sep 8, 2020

RogerRiggs commented Sep 8, 2020

openjdk bot commented Sep 8, 2020

mlbridge bot commented Sep 8, 2020 •

edited

Loading

dfuch Sep 8, 2020 •

edited

Loading

dfuch Sep 8, 2020

LanceAndersen left a comment

openjdk bot commented Sep 8, 2020 •

edited

Loading

dfuch left a comment

dfuch Sep 8, 2020

stuart-marks Sep 8, 2020

RogerRiggs commented Sep 9, 2020 via email

RogerRiggs commented Sep 10, 2020

openjdk bot commented Sep 10, 2020

8252830: Correct missing javadoc comments in java.rmi module #79

8252830: Correct missing javadoc comments in java.rmi module #79

Conversation

RogerRiggs commented Sep 8, 2020 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Sep 8, 2020

RogerRiggs commented Sep 8, 2020

openjdk bot commented Sep 8, 2020

RogerRiggs commented Sep 8, 2020

openjdk bot commented Sep 8, 2020

mlbridge bot commented Sep 8, 2020 • edited Loading

Webrevs

dfuch Sep 8, 2020 • edited Loading

Choose a reason for hiding this comment

dfuch Sep 8, 2020

Choose a reason for hiding this comment

LanceAndersen left a comment

Choose a reason for hiding this comment

openjdk bot commented Sep 8, 2020 • edited Loading

dfuch left a comment

Choose a reason for hiding this comment

dfuch Sep 8, 2020

Choose a reason for hiding this comment

stuart-marks Sep 8, 2020

Choose a reason for hiding this comment

RogerRiggs commented Sep 9, 2020 via email

RogerRiggs commented Sep 10, 2020

openjdk bot commented Sep 10, 2020

RogerRiggs commented Sep 8, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 8, 2020 •

edited

Loading

dfuch Sep 8, 2020 •

edited

Loading

openjdk bot commented Sep 8, 2020 •

edited

Loading