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

8274075: Fix miscellaneous typos in java.base #5610

Closed
wants to merge 5 commits into from

Conversation

pavelrappo
Copy link
Member

@pavelrappo pavelrappo commented Sep 21, 2021


Progress

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

Issue

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5610

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 21, 2021

👋 Welcome back prappo! 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
Copy link

@openjdk openjdk bot commented Sep 21, 2021

@pavelrappo The following labels will be automatically applied to this pull request:

  • core-libs
  • security

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.

@openjdk openjdk bot added security core-libs rfr labels Sep 21, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 21, 2021

Webrevs

dfuch
dfuch approved these changes Sep 21, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 21, 2021

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

8274075: Fix miscellaneous typos in java.base

Reviewed-by: dfuchs, darcy, iris, lancea, bpb

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

  • 57fe11c: 8274120: [JVMCI] CompileBroker should resolve parameter types for JVMCI compiles
  • 81d4164: 8272759: (fc) java/nio/channels/FileChannel/Transfer2GPlus.java failed in timeout
  • da38ced: 8271602: Add Math.ceilDiv() family parallel to Math.floorDiv() family
  • d39aad9: 8273924: ArrayIndexOutOfBoundsException thrown in java.util.JapaneseImperialCalendar.add()
  • c9de806: 8274039: codestrings gtest fails when hsdis is present
  • 33df388: 8274003: ProcessHandleImpl.Info toString has an if check which is always true
  • 0a36163: 8272600: (test) Use native "sleep" in Basic.java
  • c6df3c9: 8274071: Clean up java.lang.ref comments and documentation
  • 71788c6: 8271287: jdk/jshell/CommandCompletionTest.java fails with "lists don't have the same size expected"
  • ba7d550: 8270139: jshell InternalError crash for import of @repeatable followed by unresolved ref
  • ... and 20 more: https://git.openjdk.java.net/jdk/compare/afd218d39a3125fcea50968edef6e6cfbacfff50...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 Sep 21, 2021
@@ -65,7 +65,7 @@
* it is generally unrelated to the abstraction provided by the upper layer.
* Further, doing so would tie the API of the upper layer to the details of
* its implementation, assuming the lower layer's exception was a checked
* exception. Throwing a "wrapped exception" (i.e., an exception containing a
* exception. Throwing a "wrapping exception" (i.e., an exception containing a
Copy link
Member

@bplb bplb Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure that this should be "wrapping" and not "wrapped?" See also for example java.security.cert.CertPathValidatorException.

Copy link
Contributor

@LanceAndersen LanceAndersen Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does seem a bit strange to say "Throwing a wrapping...."

Copy link
Member Author

@pavelrappo pavelrappo Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have two exceptions A and B, such that B is the cause of A, then A is the wrapping exception (the one that wraps or the wrapper) and B is the wrapped exception (the one that is being wrapped or the wrappee).

I noticed that the sentence was conflicting: it started with the "wrapped" exception, but then in the "i.e." commentary in parentheses proceeded with the wrappee exception. To resolve that conflict, I proposed to rephrase the first part of that sentence. FWIW, my original suggestion elsewhere was to use "wrapper".

Copy link
Member

@bplb bplb Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Subjectively, "wrapping exception" would seem to be an exception in the process of wrapping something.

Copy link
Member Author

@pavelrappo pavelrappo Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would "wrappER" be better?

Copy link
Member Author

@pavelrappo pavelrappo Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can either revert this part of the change or rephrase it. Mind you, rephrasing might prove tricky because of non-local changes it might introduce. There's one more occurrence of "wrapped exception" in this file:

* Note the presence of lines containing the characters {@code "..."}.
* These lines indicate that the remainder of the stack trace for this
* exception matches the indicated number of frames from the bottom of the
* stack trace of the exception that was caused by this exception (the
* "enclosing" exception). This shorthand can greatly reduce the length
* of the output in the common case where a wrapped exception is thrown
* from same method as the "causative exception" is caught. The above

Copy link
Member

@bplb bplb Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of "common case where a wrapped exception is thrown from same method" could one write "common case where an enclosing exception is thrown from the same method"?

Copy link
Member

@dholmes-ora dholmes-ora Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that we don't throw the "wrapped exception" we throw the exception that wraps it. The "wrapped exception" is the original cause. The wording as presented now is correct in that regard. You could also say "Throwing a wrapper exception (i.e. one that has a cause)" - I think both are grammatically correct.

The later text "where a wrapped exception is thrown from the same method" is again incorrect because the wrapped exception (the cause) is not what gets thrown. But I find that whole sentence rather jarring anyway - I'm not certain exactly what it means.

Copy link
Member Author

@pavelrappo pavelrappo Sep 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm inclined to revert this change on Throwable.java:68, because of the lack of consensus. It clearly is not a simple typo.

Copy link
Member Author

@pavelrappo pavelrappo Sep 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted in 57b71c5.

Copy link
Contributor

@LanceAndersen LanceAndersen left a comment

Overall looks like a good clean up. just not sure of "wrapping" vs. "wrapped" given the sentence starts with "Throwing".

@@ -65,7 +65,7 @@
* it is generally unrelated to the abstraction provided by the upper layer.
* Further, doing so would tie the API of the upper layer to the details of
* its implementation, assuming the lower layer's exception was a checked
* exception. Throwing a "wrapped exception" (i.e., an exception containing a
* exception. Throwing a "wrapping exception" (i.e., an exception containing a
Copy link
Contributor

@LanceAndersen LanceAndersen Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does seem a bit strange to say "Throwing a wrapping...."

pavelrappo added 2 commits Sep 22, 2021
JDK predominantly uses "non-whitespace". There's only one more use of "non-white space", in com/sun/tools/javac/tree/DocTreeMaker.java:716, which will go away soon.
@pavelrappo
Copy link
Member Author

@pavelrappo pavelrappo commented Sep 22, 2021

Please re-review this change since I've added two commits.

(Spotted by Brian Burkhalter.)
bplb
bplb approved these changes Sep 22, 2021
@pavelrappo
Copy link
Member Author

@pavelrappo pavelrappo commented Sep 23, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Sep 23, 2021

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

  • 8b833bb: 8274048: IGV: Replace usages of Collections.sort with List.sort call
  • a74c099: 8252842: Extend jmap to support parallel heap dump
  • 2166ed1: 8273894: ConcurrentModificationException raised every time ReferralsCache drops referral
  • 1c6fa11: 8273979: move some os time related functions to os_posix for POSIX platforms
  • 45adc92: 8273578: javax/swing/JMenu/4515762/bug4515762.java fails on macOS 12
  • 0fbbe4c: 8274033: Some tier-4 CDS EpsilonGC tests throw OOM
  • 9d3379b: 8267356: AArch64: Vector API SVE codegen support
  • 6031388: 8273714: jdk/jfr/api/consumer/TestRecordedFrame.java still times out after JDK-8273047
  • 8821b00: 8205137: Remove Applet support from SwingSet2
  • 57fe11c: 8274120: [JVMCI] CompileBroker should resolve parameter types for JVMCI compiles
  • ... and 29 more: https://git.openjdk.java.net/jdk/compare/afd218d39a3125fcea50968edef6e6cfbacfff50...master

Your commit was automatically rebased without conflicts.

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

@openjdk openjdk bot commented Sep 23, 2021

@pavelrappo Pushed as commit 8799856.

💡 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
Labels
core-libs integrated security
7 participants