-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
8252537: Updated @exception with @throws #95
Conversation
👋 Welcome back vsharma! A progress list of the required criteria for merging this PR into |
@Vipin-Sharma 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 |
/issue 8252536,8252539,8252540,8252541 |
@Vipin-Sharma Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: |
Webrevs
|
Mailing list message from serguei.spitsyn at oracle.com on serviceability-dev: An HTML attachment was scrubbed... |
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.
Please re-indent the @throws lines to add 3 spaces after so that the comments are aligned as they were previously.
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've only looked at the management files. They look good in general.
src/java.management/share/classes/java/lang/management/ClassLoadingMXBean.java
108 * @throws java.lang.SecurityException if a security manager
109 * exists and the caller does not have
110 * ManagementPermission("control").
src/java.management/share/classes/java/lang/management/MemoryMXBean.java
286 * @throws java.lang.SecurityException if a security manager
287 * exists and the caller does not have
288 * ManagementPermission("control").
Could you, please, fix the indentation?
@RogerRiggs I understand your point and will update PR with correct indentation.
I will update PR to make sure the indentation looks same as before and there is no change in javadoc. |
HI Vipin,
Correct, a better description is "fix the indentation".
I mnetioned 3 because that was the difference in length between
"exception" and "throws".
Thanks for the followup, Roger
…On 9/15/20 3:14 PM, Vipin Sharma wrote:
@RogerRiggs
<https://urldefense.com/v3/__https://github.com/RogerRiggs__;!!GqivPVa7Brio!IQp3U9OccSzrjwI7nzJl491MGc8URG355lsnpdvFTIOGQcgmRGlpC-Kpz3kh64J6$>
I understand your point and will update PR with correct indentation.
But I think adding 3 spaces after throws may not be right for all cases.
For example when
1. Another tag in same method is using only 1 space.
2. In some cases (e.g. free method of Blob.java) we had a mix of
throws and exception in the same method both with one space after.
Here after adding 3 spaces throws tags will have the different
number of spaces and indentation will not be same as before.
I will update PR to make sure the indentation looks same as before and
there is no change in javadoc.
Please tell me in case my understnding is not correct here.
—
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/95*issuecomment-692922280__;Iw!!GqivPVa7Brio!IQp3U9OccSzrjwI7nzJl491MGc8URG355lsnpdvFTIOGQcgmRGlpC-Kpz78EqgGt$>,
or unsubscribe
<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AAIRSVAMITTR62U3YVETCM3SF64IFANCNFSM4RDCQ4MA__;!!GqivPVa7Brio!IQp3U9OccSzrjwI7nzJl491MGc8URG355lsnpdvFTIOGQcgmRGlpC-Kpz1RGWzIm$>.
|
|
There was something wrong with the way I updated this PR, please ignore this. I will try to correct this or will close this PR and create a new one. |
e995346
to
37b6936
Compare
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've reviewed the management files only. They look good.
@Vipin-Sharma 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 more 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 178 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. 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 (@RogerRiggs, @sspitsyn, @LanceAndersen) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
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.
Overall the changes look OK. in the java.sql set of classes, please updated the modified statements to also use {@code}
* if a database access error occurs or | ||
* this method is called on a closed <code>CallableStatement</code> | ||
* @exception SQLFeatureNotSupportedException if <code>sqlType</code> is | ||
* @throws SQLFeatureNotSupportedException if <code>sqlType</code> is | ||
* a <code>ARRAY</code>, <code>BLOB</code>, <code>CLOB</code>, | ||
* <code>DATALINK</code>, <code>JAVA_OBJECT</code>, <code>NCHAR</code>, | ||
* <code>NCLOB</code>, <code>NVARCHAR</code>, <code>LONGNVARCHAR</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.
Please change to use {@code) as part of the update
* if a database access error occurs or | ||
* this method is called on a closed <code>CallableStatement</code> | ||
* @exception SQLFeatureNotSupportedException if <code>sqlType</code> is | ||
* @throws SQLFeatureNotSupportedException if <code>sqlType</code> is | ||
* a <code>ARRAY</code>, <code>BLOB</code>, <code>CLOB</code>, | ||
* <code>DATALINK</code>, <code>JAVA_OBJECT</code>, <code>NCHAR</code>, | ||
* <code>NCLOB</code>, <code>NVARCHAR</code>, <code>LONGNVARCHAR</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.
Same comment regarding {@code}
Hi @LanceAndersen, it was suggested by @RogerRiggs also. I have created a separate bug JDK-8253612 to fix this for all java.sql and all other core-libs classes. |
Hi Vipin, I agree overall that to replace all of places in the JDK with {@code} where needed should be a separate task with an umbrella issue with a subtask for each module. Unless there are a large number of other exceptions that are being modified and require updates for {@code} I would prefer to have it done as part of this set of changes |
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 changes look good. Thank you for addressing this.
/integrate |
@Vipin-Sharma |
/integrate |
@LanceAndersen Only the author (@Vipin-Sharma) is allowed to issue the |
/sponsor |
@LanceAndersen @Vipin-Sharma Since your change was applied there have been 185 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit dffe9db. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
* Use pthread_set_name_np to set the thread name on FreeBSD Originally authored by: bra@fsn.hu (openjdk8) Forward ported by: huanghwh Issue openjdk#95
Updated @exception with @throws for core-libs, it fixes all open sub-tasks of JDK-8252536.
Open Subtasks part of this fix are:
Previous conversation on this:
https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-September/068540.html
Progress
Issues
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/95/head:pull/95
$ git checkout pull/95