8290775: Some doc errors in DerOutputStream.java #9585

jquanC · 2022-07-21T08:53:31Z

There are some doc errors in sun.security.util.DerOutputStream, like the followings,

/**
 * Private helper routine for writing DER encoded string values.
 * @param s the string to write
 * @param stringTag one of the DER string tags that indicate which
 * encoding should be used to write the string out.
 * @param enc the name of the encoder that should be used corresponding
 * to the above tag.
 */
private void writeString(String s, byte stringTag, Charset charset) throws IOException

The parameter is charset, but not enc.

/**
 * Marshals a DER integer on the output stream.
 *
 * @param i the integer in bytes, equivalent to BigInteger::toByteArray.
 */
public void putInteger(byte[] buf) throws IOException {

The parameter is buf, but not i.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8290775: Some doc errors in DerOutputStream.java

Reviewers

Xue-Lei Andrew Fan (@XueleiFan - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9585/head:pull/9585
$ git checkout pull/9585

Update a local copy of the PR:
$ git checkout pull/9585
$ git pull https://git.openjdk.org/jdk pull/9585/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9585

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9585.diff

bridgekeeper · 2022-07-21T08:54:22Z

Hi @jquanC, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user jquanC" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

openjdk · 2022-07-21T08:56:57Z

⚠️ @jquanC a branch with the same name as the source branch for this pull request (master) is present in the target repository. If you eventually integrate this pull request then the branch master in your personal fork will diverge once you sync your personal fork with the upstream repository.

To avoid this situation, create a new branch for your changes and reset the master branch. You can do this by running the following commands in a local repository for your personal fork. Note: you do not have to name the new branch NEW-BRANCH-NAME.

$ git checkout -b NEW-BRANCH-NAME
$ git branch -f master 3582fd9e93d9733c6fdf1f3848e0a093d44f6865
$ git push -f origin master

Then proceed to create a new pull request with NEW-BRANCH-NAME as the source branch and close this one.

openjdk · 2022-07-21T08:57:39Z

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

security

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.

mlbridge · 2022-07-21T21:18:28Z

Webrevs

XueleiFan

Thanks for the update. However, I have concerns about the update of parameter descriptions.

XueleiFan · 2022-07-21T21:49:43Z

src/java.base/share/classes/sun/security/util/DerOutputStream.java

@@ -175,7 +175,7 @@ public void putInteger(BigInteger i) throws IOException {
    /**
     * Marshals a DER integer on the output stream.
     *
-     * @param i the integer in bytes, equivalent to BigInteger::toByteArray.
+     * @param buf buffered data, which must be DER-encoded


The information of the parameter, "the integer in bytes, equivalent to BigInteger::toByteArray", may be not necessary to be updated, which is right I think.

XueleiFan · 2022-07-21T21:51:26Z

src/java.base/share/classes/sun/security/util/DerOutputStream.java

@@ -461,8 +461,8 @@ public void putGeneralString(String s) throws IOException {
     * @param s the string to write
     * @param stringTag one of the DER string tags that indicate which
     * encoding should be used to write the string out.
-     * @param enc the name of the encoder that should be used corresponding
-     * to the above tag.
+     * @param charset the specified character set encodes a string into a


I think it may be not necessary to update the parameter description.

jquanC · 2022-07-22T01:43:06Z

Thanks for your careful check! On the first point, I am not careful enough. As you said, there is no need to modify it cause it is quite clear at the end of the sentence. On the second point, I have some doubts. 1) Here don't need to add ***@***.*** charset" because it's clear to everyone? ***@***.*** enc" does not seem to be used in the method. Shouldn't it be deleted? Looking forward to your guidance. Thanks again!

…

------------------ 原始邮件 ------------------ 发件人: "openjdk/jdk" ***@***.***>; 发送时间: 2022年7月22日(星期五) 凌晨5:53 ***@***.***>; ***@***.******@***.***>; 主题: Re: [openjdk/jdk] 8290775: Some doc errors in DerOutputStream.java (PR #9585) @XueleiFan requested changes on this pull request. Thanks for the update. However, I have concerns about the update os parameter descriptions. In src/java.base/share/classes/sun/security/util/DerOutputStream.java: > @@ -175,7 +175,7 @@ public void putInteger(BigInteger i) throws IOException { /** * Marshals a DER integer on the output stream. * - * @param i the integer in bytes, equivalent to BigInteger::toByteArray. + * @param buf buffered data, which must be DER-encoded The information of the parameter, "the integer in bytes, equivalent to BigInteger::toByteArray", may be not necessary to be updated, which is right I think. In src/java.base/share/classes/sun/security/util/DerOutputStream.java: > @@ -461,8 +461,8 @@ public void putGeneralString(String s) throws IOException { * @param s the string to write * @param stringTag one of the DER string tags that indicate which * encoding should be used to write the string out. - * @param enc the name of the encoder that should be used corresponding - * to the above tag. + * @param charset the specified character set encodes a string into a I think it may be not necessary to update the parameter description. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: ***@***.***>

XueleiFan · 2022-07-22T05:25:41Z

On the second point, I have some doubts. 1) Here don't need to add @.*** charset" because it's clear to everyone? @.*** enc" does not seem to be used in the method. Shouldn't it be deleted?

Sorry for the confusing.

I think it is fine to update the "param enc" to "param charset", but it may be not necessary to update the parameter description. With "parameter description", I refer to this part, "the name of the encoder that should be used corresponding to the above tag".

The parameter description may be still confusing to someone, as the 'the name of the encoder' is not well-defined here. Maybe, it could be simplified as "the charset that ..."

Here is the proposed update:

-     * @param enc the name of the encoder that should be used corresponding
-    * to the above tag.
+    * @param charset the specified character set encodes a string into a
+    * sequence of bytes using

I may use an update like :

-     * @param enc the name of the encoder that should be used corresponding
-    * to the above tag.
+    * @param charset the charset that is should used corresponding to the
+    * above tag.

…vement

wangweij · 2022-07-22T12:44:54Z

s/is should used/should be used/.

jquanC · 2022-07-22T13:13:23Z

s/is should used/should be used/.

Thanks for your advice!
Yes, the passive voice should be used.
I may offer an update like:

@param charset the charset should be used corresponding to the above tag.

…ense problem of expression

XueleiFan · 2022-07-22T15:11:43Z

src/java.base/share/classes/sun/security/util/DerOutputStream.java

@@ -461,8 +461,8 @@ public void putGeneralString(String s) throws IOException {
     * @param s the string to write
     * @param stringTag one of the DER string tags that indicate which
     * encoding should be used to write the string out.
-     * @param enc the name of the encoder that should be used corresponding
-     * to the above tag.
+     * @param charset the charset should be used corresponding to the


I may suggest to add 'that' back. Without it, the meaning of the sentence may be different.

... the charset should be used corresponding to the above tag. vs ... the charset **that** should be used corresponding to the above tag.

Thanks again for all the above suggestions sincerely!
To avoid adding your troubles, I should end up using the following expression:

the charset that should be used corresponding to the above tag.

I'm trying to understand the difference between the two.
The first sentence may be understood as "the process of using charset" according to the above tag.
The second sentence(use that) can better express "the choice of charset" needs to be based on the above tag.
Is this correct?

A spec for an argument should be a noun. You can add attributes to describe it in more detail, which can be an adjective, or even a clause. This is what's shown in the 2nd line (I cannot call it a sentence because it's just a noun with an attributive clause). On the other hand, the 1st is a full sentence and it's not what we need here.

A spec for an argument should be a noun. You can add attributes to describe it in more detail, which can be an adjective, or even a clause. This is what's shown in the 2nd line (I cannot call it a sentence because it's just a noun with an attributive clause). On the other hand, the 1st is a full sentence and it's not what we need here.

Thanks so much for your detailed explanation.
I get it now.

…ssion

wangweij · 2022-07-22T17:39:20Z

Two comments:

You reverted the s/i/buf/ change in putInteger.
The other constructor also uses an DER output steam. Since you already fixed one, please take care of this one as well.

jquanC · 2022-07-23T01:42:35Z

Two comments:

You reverted the s/i/buf/ change in putInteger.

The other constructor also uses an DER output steam. Since you already fixed one, please take care of this one as well.

Got it.
For the first, "the integer in bytes, equivalent to BigInteger::toByteArray", I adopted another reviewer's advice and I reverted it. But I may have misunderstood what he meant. The correct update should look like this：

@param buf the integer in bytes, equivalent to BigInteger::toByteArray.

For the second, I will take care of that.
Thanks again! I will offer an update later.

…cation and improve an expression according to the specification

XueleiFan

Looks good to me. Thanks!

openjdk · 2022-07-23T05:33:56Z

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

8290775: Some doc errors in DerOutputStream.java

Reviewed-by: xuelei

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

ec53fba: 8292361: Build failure on PPC64 BE after JDK-8290840
4cc6cb9: 8290041: ModuleDescriptor.hashCode is inconsistent
d1edda8: 8292338: aarch64: Use cbnz instruction in gen_continuation_enter when possible
21f4eb2: 6521141: DebugGraphics NPE @ setFont();
6e6ae59: 8292286: Convert PlaceholderTable to ResourceHashtable
ea2c82e: 8291949: Unexpected extending of SupportedGroups
b5707b0: 8292196: Reduce runtime of java.util.regex microbenchmarks
b00eede: 8291511: Redefinition of EXIT_FAILURE in libw2k_lsa_auth
3a09077: 8291337: Reduce runtime of vm.lamdba microbenchmarks
dd2034b: 8291972: Fix double copy of arguments when thawing two interpreted frames
... and 247 more: https://git.openjdk.org/jdk/compare/3582fd9e93d9733c6fdf1f3848e0a093d44f6865...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@XueleiFan) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

jquanC · 2022-07-23T05:37:16Z

Looks good to me. Thanks!

Cheers and appreciation! And I have learned more from your guidance.

wangweij · 2022-08-16T14:27:43Z

@jquanC Please type/integrate if you want to get the fix included in OpenJDK.

jquanC · 2022-08-16T14:35:49Z

/integrate

jquanC · 2022-08-16T14:36:39Z

@jquanC Please type/integrate if you want to get the fix included in OpenJDK.

Thanks for your reminder!
I just marked it, please check if it is correct.

openjdk · 2022-08-16T14:37:26Z

@jquanC
Your change (at version 14b2802) is now ready to be sponsored by a Committer.

wangweij · 2022-08-16T14:44:34Z

/sponsor

openjdk · 2022-08-16T14:46:25Z

Going to push as commit 3e12241.
Since your change was applied there have been 257 commits pushed to the master branch:

ec53fba: 8292361: Build failure on PPC64 BE after JDK-8290840
4cc6cb9: 8290041: ModuleDescriptor.hashCode is inconsistent
d1edda8: 8292338: aarch64: Use cbnz instruction in gen_continuation_enter when possible
21f4eb2: 6521141: DebugGraphics NPE @ setFont();
6e6ae59: 8292286: Convert PlaceholderTable to ResourceHashtable
ea2c82e: 8291949: Unexpected extending of SupportedGroups
b5707b0: 8292196: Reduce runtime of java.util.regex microbenchmarks
b00eede: 8291511: Redefinition of EXIT_FAILURE in libw2k_lsa_auth
3a09077: 8291337: Reduce runtime of vm.lamdba microbenchmarks
dd2034b: 8291972: Fix double copy of arguments when thawing two interpreted frames
... and 247 more: https://git.openjdk.org/jdk/compare/3582fd9e93d9733c6fdf1f3848e0a093d44f6865...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-08-16T14:46:40Z

@wangweij @jquanC Pushed as commit 3e12241.

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

jquanC · 2022-10-11T08:38:38Z

Thanks for your valuable advice. I will continue to improve. Best wishes!

…

------------------ 原始邮件 ------------------ 发件人: "openjdk/jdk" ***@***.***>; 发送时间: 2022年7月22日(星期五) 中午1:25 ***@***.***>; ***@***.******@***.***>; 主题: Re: [openjdk/jdk] 8290775: Some doc errors in DerOutputStream.java (PR #9585) On the second point, I have some doubts. 1) Here don't need to add @.*** charset" because it's clear to everyone? @.*** enc" does not seem to be used in the method. Shouldn't it be deleted? Sorry for the confusing. I think it is fine to update the "param enc" to "param charset", but it may be not necessary to update the parameter description. With "parameter description", I refer to this part, "the name of the encoder that should be used corresponding to the above tag". The parameter description may be still confusing to someone, as the 'the name of the encoder' is not well-defined here. Maybe, it could be simplified as "the charset that ..." Here is the proposed update: * @param enc the name of the encoder that should be used corresponding to the above tag. @param charset the specified character set encodes a string into a sequence of bytes using I may use an update like : * @param enc the name of the encoder that should be used corresponding to the above tag. @param charset the charset that is should used corresponding to the above tag. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: ***@***.***>

8290775: Some doc errors in DerOutputStream.java

c6f17c8

bridgekeeper bot added the oca Needs verification of OCA signatory status label Jul 21, 2022

openjdk bot added the security security-dev@openjdk.org label Jul 21, 2022

bridgekeeper bot removed the oca Needs verification of OCA signatory status label Jul 21, 2022

openjdk bot added the rfr Pull request is ready for review label Jul 21, 2022

XueleiFan suggested changes Jul 21, 2022

View reviewed changes

8290775: Some doc errors in DerOutputStream.java Update: better impro…

c2a0a8b

…vement

8290775: Some doc errors in DerOutputStream.java Update:correct the t…

c2a0530

…ense problem of expression

XueleiFan reviewed Jul 22, 2022

View reviewed changes

8290775: Some doc errors in DerOutputStream.java Update: better expre…

b8b25f2

…ssion

8290775: Some doc errors in DerOutputStream.java Update: add a modifi…

14b2802

…cation and improve an expression according to the specification

XueleiFan approved these changes Jul 23, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 23, 2022

openjdk bot added the sponsor Pull request is ready to be sponsored label Aug 16, 2022

openjdk bot added the integrated Pull request has been integrated label Aug 16, 2022

openjdk bot closed this Aug 16, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Aug 16, 2022

8290775: Some doc errors in DerOutputStream.java #9585

8290775: Some doc errors in DerOutputStream.java #9585

jquanC commented Jul 21, 2022 •

edited by openjdk bot

bridgekeeper bot commented Jul 21, 2022

openjdk bot commented Jul 21, 2022

openjdk bot commented Jul 21, 2022

mlbridge bot commented Jul 21, 2022 •

edited

XueleiFan left a comment •

edited

XueleiFan Jul 21, 2022

XueleiFan Jul 21, 2022

jquanC commented Jul 22, 2022 via email

XueleiFan commented Jul 22, 2022 •

edited

wangweij commented Jul 22, 2022

jquanC commented Jul 22, 2022

XueleiFan Jul 22, 2022

jquanC Jul 22, 2022 •

edited

wangweij Jul 22, 2022

jquanC Jul 23, 2022

wangweij commented Jul 22, 2022 •

edited

jquanC commented Jul 23, 2022 •

edited

XueleiFan left a comment

openjdk bot commented Jul 23, 2022 •

edited

jquanC commented Jul 23, 2022

wangweij commented Aug 16, 2022

jquanC commented Aug 16, 2022

jquanC commented Aug 16, 2022

openjdk bot commented Aug 16, 2022

wangweij commented Aug 16, 2022

openjdk bot commented Aug 16, 2022

openjdk bot commented Aug 16, 2022

jquanC commented Oct 11, 2022 via email

8290775: Some doc errors in DerOutputStream.java #9585

8290775: Some doc errors in DerOutputStream.java #9585

Conversation

jquanC commented Jul 21, 2022 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jul 21, 2022

openjdk bot commented Jul 21, 2022

openjdk bot commented Jul 21, 2022

mlbridge bot commented Jul 21, 2022 • edited

Webrevs

XueleiFan left a comment • edited

Choose a reason for hiding this comment

XueleiFan Jul 21, 2022

Choose a reason for hiding this comment

XueleiFan Jul 21, 2022

Choose a reason for hiding this comment

jquanC commented Jul 22, 2022 via email

XueleiFan commented Jul 22, 2022 • edited

wangweij commented Jul 22, 2022

jquanC commented Jul 22, 2022

XueleiFan Jul 22, 2022

Choose a reason for hiding this comment

jquanC Jul 22, 2022 • edited

Choose a reason for hiding this comment

wangweij Jul 22, 2022

Choose a reason for hiding this comment

jquanC Jul 23, 2022

Choose a reason for hiding this comment

wangweij commented Jul 22, 2022 • edited

jquanC commented Jul 23, 2022 • edited

XueleiFan left a comment

Choose a reason for hiding this comment

openjdk bot commented Jul 23, 2022 • edited

jquanC commented Jul 23, 2022

wangweij commented Aug 16, 2022

jquanC commented Aug 16, 2022

jquanC commented Aug 16, 2022

openjdk bot commented Aug 16, 2022

wangweij commented Aug 16, 2022

openjdk bot commented Aug 16, 2022

openjdk bot commented Aug 16, 2022

jquanC commented Oct 11, 2022 via email

jquanC commented Jul 21, 2022 •

edited by openjdk bot

mlbridge bot commented Jul 21, 2022 •

edited

XueleiFan left a comment •

edited

XueleiFan commented Jul 22, 2022 •

edited

jquanC Jul 22, 2022 •

edited

wangweij commented Jul 22, 2022 •

edited

jquanC commented Jul 23, 2022 •

edited

openjdk bot commented Jul 23, 2022 •

edited