Skip to content
This repository has been archived by the owner. It is now read-only.

JDK-8268972: Add default impl for recent new Reporter.print method #92

Conversation

jonathan-gibbons
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons commented Jun 17, 2021

Please review a simple fix to add a default implementation for a recent new method on the Reporter interface. (It was an oversight that the default implementation was not provided in the original work.)


Progress

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

Issue

  • JDK-8268972: Add default impl for recent new Reporter.print method

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 92

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 17, 2021

👋 Welcome back jjg! 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 openjdk bot added the rfr Pull request is ready for review label Jun 17, 2021
@jonathan-gibbons jonathan-gibbons changed the title JDK-8268973: Add default impl for recent new Reporter.print method JDK-8268972: Add default impl for recent new Reporter.print method Jun 17, 2021
@jonathan-gibbons
Copy link
Contributor Author

jonathan-gibbons commented Jun 17, 2021

/csr

@openjdk
Copy link

openjdk bot commented Jun 17, 2021

@jonathan-gibbons The following label will be automatically applied to this pull request:

  • javadoc

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.

@openjdk openjdk bot added javadoc javadoc-dev@openjdk.java.net csr Pull request needs approved CSR before integration labels Jun 17, 2021
@openjdk
Copy link

openjdk bot commented Jun 17, 2021

@jonathan-gibbons this pull request will not be integrated until the CSR request JDK-8268973 for issue JDK-8268972 has been approved.

@mlbridge
Copy link

mlbridge bot commented Jun 17, 2021

Webrevs

Copy link
Member

@pavelrappo pavelrappo left a comment

Synchronize with the respective CSR.

@@ -94,6 +94,12 @@
* The positions are all 0-based character offsets from the beginning of content of the file.
* The positions should satisfy the relation {@code start <= pos <= end}.
*
* @implNote
Copy link
Member

@pavelrappo pavelrappo Jun 17, 2021

Choose a reason for hiding this comment

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

Should be @implSpec instead: both here and in the respective CSR.

@@ -94,6 +94,12 @@
* The positions are all 0-based character offsets from the beginning of content of the file.
* The positions should satisfy the relation {@code start <= pos <= end}.
*
* @implNote
* This implementation throws {@code UnsupportedOperationException}.
Copy link
Member

@pavelrappo pavelrappo Jun 17, 2021

Choose a reason for hiding this comment

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

I'd suggest rephrasing this as follows: "This implementation always throws..." both here and in the respective CSR.

* This implementation throws {@code UnsupportedOperationException}.
* The implementation provided by the {@code javadoc} tool to
* {@link Doclet#init(Locale, Reporter) initialize} a doclet
* overrides this implementation.
Copy link
Member

@pavelrappo pavelrappo Jun 17, 2021

Choose a reason for hiding this comment

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

Overrides how?

Copy link
Contributor Author

@jonathan-gibbons jonathan-gibbons Jun 17, 2021

Choose a reason for hiding this comment

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

...per the documented spec. I tried writing The implementation .... always generates an appropriate diagnostic but that's not true if an exception is thrown, and writing The implementation ... does not throw UOE seems weird.

Copy link
Member

@pavelrappo pavelrappo Jun 17, 2021

Choose a reason for hiding this comment

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

The difficulty you have with ironing out this wording wrinkle suggests that the behavior of the Reporter provided to Doclet.init should be described in Doclet.init rather than here. The same goes for other default methods of Reporter.

@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Jun 22, 2021
@openjdk
Copy link

openjdk bot commented Jun 22, 2021

@jonathan-gibbons 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:

8268972: Add default impl for recent new Reporter.print method

Reviewed-by: prappo

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

  • 01f12fb: 8266631: StandardJavaFileManager: getJavaFileObjects() impl violates the spec
  • 6b14c8a: 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling
  • ef4ba22: 8268349: Provide clear run-time warnings about Security Manager deprecation
  • 4099810: 8268293: VectorAPI cast operation on mask and shuffle is broken
  • e2d7ec3: 8267100: [BACKOUT] JDK-8196415 Disable SHA-1 Signed JARs
  • d3ad8cd: 8268672: C2: assert(!loop->is_member(u_loop)) failed: can be in outer loop or out of both loops only
  • f25e719: 8268717: Upstream: 8268673: Stack walk across optimized entry frame on fresh native thread fails
  • 22ebd19: 8268362: [REDO] C2 crash when compile negative Arrays.copyOf length after loop
  • f8df953: 8268702: JFR diagnostic commands lack argument descriptors when viewed using Platform MBean Server
  • c294ae4: 8267042: bug in monitor locking/unlocking on ARM32 C1 due to uninitialized BasicObjectLock::_displaced_header
  • ... and 27 more: https://git.openjdk.java.net/jdk17/compare/0011b52ee81d88b023f771432abc079c64266578...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 Pull request is ready to be integrated label Jun 22, 2021
@jonathan-gibbons
Copy link
Contributor Author

jonathan-gibbons commented Jun 25, 2021

/integrate

@openjdk
Copy link

openjdk bot commented Jun 25, 2021

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

  • 7ab1285: 8267610: NPE at at jdk.compiler/com.sun.tools.javac.jvm.Code.emitop
  • cfa6a99: 8269316: ProblemList vmTestbase/vm/mlvm/mixed/stress/regression/b6969574/INDIFY_Test.java on Linux-X64 -Xcomp
  • 22d8675: 8269315: ProblemList javax/swing/JFileChooser/FileSystemView/SystemIconTest.java on Win-X64
  • 443a79a: 8269314: ProblemList serviceability/dcmd/gc/RunFinalizationTest.java on Win-X64 and linux-aarch64
  • 424cc50: 8269307: ProblemList java/awt/KeyboardFocusmanager/TypeAhead/ButtonActionKeyTest/ButtonActionKeyTest.java on win-x64
  • 63bcd33: 8269246: Scoped ByteBuffer vector access
  • 3fb28d3: 8269218: GaloisCounterMode.overlapDetection misses the JDK-8263436 fix again
  • d3d3b22: 8269265: ProblemList serviceability/sa/TestJmapCoreMetaspace.java with ZGC
  • 0c3fc27: 8268482: compiler/intrinsics/VectorizedMismatchTest.java failed with failed: length in range
  • a30141d: 8269179: Crash in TestMacroLogicVector::testSubWordBoolean: assert(_base >= VectorMask && _base <= VectorZ) failed: Not a Vector
  • ... and 50 more: https://git.openjdk.java.net/jdk17/compare/0011b52ee81d88b023f771432abc079c64266578...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Jun 25, 2021
@openjdk 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 Jun 25, 2021
@openjdk
Copy link

openjdk bot commented Jun 25, 2021

@jonathan-gibbons Pushed as commit 44691cc.

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
integrated Pull request has been integrated javadoc javadoc-dev@openjdk.java.net
2 participants