8248383: Clarify java.io.Reader.read(char[], ...) behavior for full array #1737

bplb · 2020-12-10T17:22:06Z

Please review this small verbiage change to specify clearly the behavior of Reader::read(char[] cbuf) when the length of cbuf is zero, and that of Reader::read(char[] cbuf, int off, int len) when len is zero.

Progress

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

Issue

JDK-8248383: Clarify java.io.Reader.read(char[], ...) behavior for full array

Reviewers

Naoto Sato (@naotoj - Reviewer) ⚠️ Review applies to 0dc2e24
Stuart Marks (@stuart-marks - Reviewer)
Roger Riggs (@RogerRiggs - Reviewer)

Download

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

…rray

bplb · 2020-12-10T17:22:14Z

/csr

bridgekeeper · 2020-12-10T17:22:47Z

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

Reviewed-by: kvn, iklam

openjdk · 2020-12-10T17:23:51Z

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

openjdk · 2020-12-10T17:24:32Z

@bplb 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 pull request command.

mlbridge · 2020-12-10T17:28:02Z

Webrevs

…w/ jdk17 Reviewed-by: mikael, hseigel, darcy

LanceAndersen

Hi Brian,

Are there existing tests that cover the behavior described in the proposed change?

bplb · 2020-12-10T18:51:39Z

Hi @LanceAndersen. I don't see any tests specific to this issue, but then there are ten Reader classes in java.io, seven of which are not abstract.

Reviewed-by: lancea

…fails against jdk17 Reviewed-by: jwilhelm

Reviewed-by: kbarrett, tschatzl, gziemski

…rray

naotoj

Looks good to me, Brian.

…into JDK-8248383-Reader-read

openjdk · 2020-12-10T23:24:56Z

⚠️ @bplb This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

bplb · 2020-12-10T23:26:00Z

For the sake of sanity I added a minimal test to verify that the behavior is in fact in accord with the added verbiage.

RogerRiggs

Somewhat odd that the files changed includes merged file from a different changeset
but seems immaterial to the changes for the issue.

RogerRiggs · 2020-12-16T14:52:39Z

test/jdk/java/io/Reader/ReadIntoZeroLengthArray.java

+ * @summary Ensure that zero is returned for read into zero length array
+ */
+public class ReadIntoZeroLengthArray {
+    private static char[] cbuf0 = new char[0];


I would encourage the use of TestNG for new tests and they provide a good DataProvider framework for multiple test cases. (And frequently they run more quickly because they can be run in the jtreg process without spawing a separate process).

I think the extra commits are because I put some commits on the master branch and then merged them into the feature branch. I believe the integration will merge them correctly.

I converted the test to TestNG. I've used it a number of times before to good effect (cf. java/nio/Buffer/BulkPutBuffer.java) but was too lazy to review the annotations again. Thanks for the suggestion.

openjdk · 2020-12-17T05:47:57Z

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

8248383: Clarify java.io.Reader.read(char[], ...) behavior for full array

Reviewed-by: naoto, smarks, rriggs

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

178c001: 8258479: Minor cleanups in VMError
c11525a: 8257772: Vectorizing clear memory operation using AVX-512 masked operations
d77b49d: 8258233: Reenable another fixed problemlisted test
513269d: 8196093: javax/swing/JComboBox/8072767/bug8072767.java fails
643ddc1: 8257882: Implement linkToNative intrinsic on AArch64
11bd7a8: Merge
ce0ab2d: 8258338: Support deprecated records
6b4b676: 8241353: NPE in ToolProvider.getSystemJavaCompiler
87644a2: 8255880: UI of Swing components is not redrawn after their internal state changed
72dfba8: 8257637: Update usage of "type" terminology in java.lang.annotation
... and 70 more: https://git.openjdk.java.net/jdk/compare/80dac5a87c63981a2588e29fe8127a6787abba47...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.

bplb · 2020-12-17T16:29:28Z

/integrate

openjdk · 2020-12-17T16:30:27Z

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

8251b74: 8257530: vmTestbase/metaspace/stressDictionary/StressDictionary.java timed out
6aa8eed: 8258415: gtest for committed memory leaks reservation
83be8a9: 8247732: validate user-input intrinsic_ids in ControlIntrinsic
178c001: 8258479: Minor cleanups in VMError
c11525a: 8257772: Vectorizing clear memory operation using AVX-512 masked operations
d77b49d: 8258233: Reenable another fixed problemlisted test
513269d: 8196093: javax/swing/JComboBox/8072767/bug8072767.java fails
643ddc1: 8257882: Implement linkToNative intrinsic on AArch64
11bd7a8: Merge
ce0ab2d: 8258338: Support deprecated records
... and 73 more: https://git.openjdk.java.net/jdk/compare/80dac5a87c63981a2588e29fe8127a6787abba47...master

Your commit was automatically rebased without conflicts.

Pushed as commit 143998e.

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

8248383: Clarify java.io.Reader.read(char[], ...) behavior for full a…

0dc2e24

…rray

8256424: Move ciSymbol::symbol_name() to ciSymbols::symbol_name()

f574056

Reviewed-by: kvn, iklam

openjdk bot added rfr Pull request is ready for review csr Pull request needs approved CSR before integration labels Dec 10, 2020

openjdk bot added the core-libs core-libs-dev@openjdk.org label Dec 10, 2020

8258054: runtime/sealedClasses/GetPermittedSubclassesTest.java fails …

3342eca

…w/ jdk17 Reviewed-by: mikael, hseigel, darcy

LanceAndersen reviewed Dec 10, 2020

View reviewed changes

Brian Burkhalter and others added 5 commits December 10, 2020 19:55

8257971: (fs) Remove unused code from WindowsPath.subpath(begin, end)

42264b2

Reviewed-by: lancea

8258056: jdk/javadoc/doclet/testHtmlTableTags/TestHtmlTableTags.java …

164c55b

…fails against jdk17 Reviewed-by: jwilhelm

8257912: Convert enum iteration to use range-based for loops

80dac5a

Reviewed-by: kbarrett, tschatzl, gziemski

8248383: Clarify java.io.Reader.read(char[], ...) behavior for full a…

04e6e62

…rray

8248383: Fix alignment of @see tags

c2a22ee

naotoj approved these changes Dec 10, 2020

View reviewed changes

bplb added 2 commits December 10, 2020 15:18

8248383: Add test ReadIntoZeroLengthArray

9364687

Merge branch 'JDK-8248383-Reader-read' of https://github.com/bplb/jdk …

d8497f0

…into JDK-8248383-Reader-read

8248383: Re-indent subclasses in @see tages

7fbc77f

stuart-marks approved these changes Dec 15, 2020

View reviewed changes

RogerRiggs approved these changes Dec 16, 2020

View reviewed changes

8248383: Convert test ReadIntoZeroLengthArray to TestNG

ab73f98

RogerRiggs approved these changes Dec 16, 2020

View reviewed changes

stuart-marks approved these changes Dec 17, 2020

View reviewed changes

openjdk bot removed the csr Pull request needs approved CSR before integration label Dec 17, 2020

openjdk bot added the ready Pull request is ready to be integrated label Dec 17, 2020

openjdk bot closed this Dec 17, 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 Dec 17, 2020

bplb deleted the JDK-8248383-Reader-read branch December 17, 2020 16:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8248383: Clarify java.io.Reader.read(char[], ...) behavior for full array #1737

8248383: Clarify java.io.Reader.read(char[], ...) behavior for full array #1737

bplb commented Dec 10, 2020 •

edited by openjdk bot

Loading

bplb commented Dec 10, 2020

bridgekeeper bot commented Dec 10, 2020

openjdk bot commented Dec 10, 2020

openjdk bot commented Dec 10, 2020

mlbridge bot commented Dec 10, 2020 •

edited

Loading

LanceAndersen left a comment

bplb commented Dec 10, 2020

naotoj left a comment

openjdk bot commented Dec 10, 2020

bplb commented Dec 10, 2020

RogerRiggs left a comment

RogerRiggs Dec 16, 2020

bplb Dec 16, 2020

openjdk bot commented Dec 17, 2020 •

edited

Loading

bplb commented Dec 17, 2020

openjdk bot commented Dec 17, 2020

8248383: Clarify java.io.Reader.read(char[], ...) behavior for full array #1737

8248383: Clarify java.io.Reader.read(char[], ...) behavior for full array #1737

Conversation

bplb commented Dec 10, 2020 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bplb commented Dec 10, 2020

bridgekeeper bot commented Dec 10, 2020

openjdk bot commented Dec 10, 2020

openjdk bot commented Dec 10, 2020

mlbridge bot commented Dec 10, 2020 • edited Loading

Webrevs

LanceAndersen left a comment

Choose a reason for hiding this comment

bplb commented Dec 10, 2020

naotoj left a comment

Choose a reason for hiding this comment

openjdk bot commented Dec 10, 2020

bplb commented Dec 10, 2020

RogerRiggs left a comment

Choose a reason for hiding this comment

RogerRiggs Dec 16, 2020

Choose a reason for hiding this comment

bplb Dec 16, 2020

Choose a reason for hiding this comment

openjdk bot commented Dec 17, 2020 • edited Loading

bplb commented Dec 17, 2020

openjdk bot commented Dec 17, 2020

bplb commented Dec 10, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 10, 2020 •

edited

Loading

openjdk bot commented Dec 17, 2020 •

edited

Loading