Re: RFR: JDK-8286783: Expand use of @inheritDoc in InputStream and OutputStream subclasses
On Sun, 15 May 2022 18:36:07 GMT, Joe Darcy wrote:
> Make the javadoc in the InputStream and OutputStream subclasses in core libs
> DRY-er by use of inheritDoc. (Any analagous changes to AudioInputStream in
> client libs will be done another a separate bug.) When the time comes, will
> do any necessary merging for the changes in[JDK-8286200.
>
> Please also review the CSR to cover the introduction of implspec and implNote
> tags: https://bugs.openjdk.java.net/browse/JDK-8286784
1. One the one hand, it's not clear to me what criterion was used for adding
`@Override` annotations. On the other hand, the more `@Override` annotations a
codebase has, the better.
2. Have you compared the resulting documentation before and after the change?
Aside from added `@implSpec` and `@implNote`, were there anything anything
different?
3. I wonder if it makes sense to also reduce the size of the doc comments by
changing explicit documentation inheritance for the `@param` and `@return` tags
to implicit one, i.e. removing the tags on the overrider's side.
src/java.base/share/classes/java/io/SequenceInputStream.java line 217:
> 215: * before the {@code close} method returns.
> 216: *
> 217: * @throws IOException {@inheritDoc}
Unexpected re-indentation; other similar cases do not have it.
-
PR: https://git.openjdk.java.net/jdk/pull/8717
Re: RFR: JDK-8286760: Update citation of "Effective Java" second edition to third edition
On Fri, 13 May 2022 21:17:22 GMT, Joe Darcy wrote: > Update reference to the latest "Effective Java" version and switch to cite > tags. Looks good! 1. Had to educate myself on the [cite element](https://html.spec.whatwg.org/multipage/text-level-semantics.html#the-cite-element). The HTML spec can amuse at times: A person's name is not the title of a work — even if people call that person a piece of work — and the element must therefore not be used to mark up people's names. 2. Indeed, items 69 and 81 are both "Prefer concurrency utilities to wait and notify". - Marked as reviewed by prappo (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8707
Integrated: 8285890: Fix some @param tags
On Fri, 29 Apr 2022 09:03:58 GMT, Pavel Rappo wrote: > * Syntactically improves a few cases from 8285676 > (https://github.com/openjdk/jdk/pull/8410) > * Fixes a few misspelled `@param` tags for elements that, although are not > included in the API Documentation, are visible in and processed by IDEs This pull request has now been integrated. Changeset: 3eb661bb Author: Pavel Rappo URL: https://git.openjdk.java.net/jdk/commit/3eb661bbe7151f3a7e949b6518f57896c2bd4136 Stats: 16 lines in 8 files changed: 0 ins; 0 del; 16 mod 8285890: Fix some @param tags Reviewed-by: dfuchs, mullan, darcy, mchung, wetmore - PR: https://git.openjdk.java.net/jdk/pull/8465
RFR: 8285890: Fix some @param tags
* Syntactically improves a few cases from 8285676 (https://github.com/openjdk/jdk/pull/8410) * Fixes a few misspelled `@param` tags for elements that, although are not included in the API Documentation, are visible in and processed by IDEs - Commit messages: - Fix misspelled `@param` - Clarify with whitespace tags from 8285676 Changes: https://git.openjdk.java.net/jdk/pull/8465/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8465=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285890 Stats: 16 lines in 8 files changed: 0 ins; 0 del; 16 mod Patch: https://git.openjdk.java.net/jdk/pull/8465.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8465/head:pull/8465 PR: https://git.openjdk.java.net/jdk/pull/8465
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v5]
On Fri, 29 Apr 2022 08:45:42 GMT, Pavel Rappo wrote: > Good catch. Sorry I missed it. This occurs in all `java/lang/ref` files. I've created a PR; feel free to review it at your convenience. - PR: https://git.openjdk.java.net/jdk/pull/8410
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v5]
On Thu, 28 Apr 2022 19:06:04 GMT, Mandy Chung wrote:
>> src/java.base/share/classes/java/lang/ref/PhantomReference.java line 48:
>>
>>> 46: * The {@link #refersTo(Object) refersTo} method can be used to test
>>> 47: * whether some object is the referent of a phantom reference.
>>> 48: * @param the type of the referent
>>
>> Shouldn't there be a space after `@param` ?
>
> Good catch. Sorry I missed it. This occurs in all `java/lang/ref` files.
> Shouldn't there be a space after `@param` ?
> Good catch. Sorry I missed it. This occurs in all `java/lang/ref` files.
I built the API documentation after this PR has been integrated and the result
was okay. I saw this output in every such case:
Type Parameters:
T - the type of the referent
javadoc is quite robust. However, for some IDEs such missing whitespace seems
significant. Not only do they highlight the `@param` tag, but the type
parameter information is missing from the rendered output.
Although it's not critical, we should fix it; I have filed JDK-8285890.
-
PR: https://git.openjdk.java.net/jdk/pull/8410
Re: RFR: 8285485: Fix typos in corelibs
On Fri, 22 Apr 2022 15:08:51 GMT, Magnus Ihse Bursie wrote:
> I ran `codespell` on modules owned by core-libs, and accepted those changes
> where it indeed discovered real typos.
>
> I will update copyright years using a script before pushing (otherwise like
> every second change would be a copyright update, making reviewing much
> harder).
>
> The long term goal here is to make tooling support for running `codespell`.
> The trouble with automating this is of course all false positives. But before
> even trying to solve that issue, all true positives must be fixed. Hence this
> PR.
Thanks for doing this so carefully. If reviewers decide that parts of this PR
need to be addressed upstream, we should probably consider contributing those
parts to respective projects.
src/java.sql.rowset/share/classes/com/sun/rowset/CachedRowSetImpl.java line 970:
> 968: * and sends a rowSetChanged event to all registered
> 969: * listeners.
> 970: * @throws SQLException if an error is occurs rolling back the RowSet
L976, same as above: "is occurs".
src/java.sql.rowset/share/classes/com/sun/rowset/JoinRowSetImpl.java line 636:
> 634: // to be INNER JOIN'ED to form a new rowset
> 635: // Compare table1.MatchColumn1.value1 == {
> table2.MatchColumn2.value1
> 636: // ... up to
> table2.MatchColumn2.valueN }
Curious: it is not some established string representation, is it?
src/java.sql.rowset/share/classes/com/sun/rowset/internal/SyncResolverImpl.java
line 664:
> 662: * and sends a {@code rowSetChanged} event to all registered
> 663: * listeners.
> 664: * @throws SQLException if an error is occurs rolling back the RowSet
L664: delete "is" in "is occurs".
src/java.sql.rowset/share/classes/com/sun/rowset/providers/RIXMLProvider.java
line 239:
> 237:
> 238: /**
> 239: * Returns the vendor name of the Reference Implementation Optimistic
L240: an optimistic _what_ provider? :-)
src/java.xml/share/classes/com/sun/xml/internal/stream/writers/XMLDOMWriterImpl.java
line 238:
> 236:
> 237: /**
> 238: * Creates a DOM Attribute @see org.w3c.dom.Node and associates it
> with the current DOM element @see org.w3c.dom.Node.
Not that it matters in this PR, but I think I should mention that `@see` tags
do not work like that. (No action needed from you.)
src/java.xml/share/classes/javax/xml/transform/Transformer.java line 127:
> 125: * namespace URI in curly braces ({}).
> 126: * @param value The value object. This can be any valid Java
> object. It is
> 127: * up to the processor to provide the proper object coersion or to
> simply
That made me pause: some systems have the notion of _type coercion_; but your
change looks right.
-
PR: https://git.openjdk.java.net/jdk/pull/8364
Re: RFR: 8285485: Fix typos in corelibs
On Fri, 22 Apr 2022 15:08:51 GMT, Magnus Ihse Bursie wrote: > I ran `codespell` on modules owned by core-libs, and accepted those changes > where it indeed discovered real typos. > > I will update copyright years using a script before pushing (otherwise like > every second change would be a copyright update, making reviewing much > harder). > > The long term goal here is to make tooling support for running `codespell`. > The trouble with automating this is of course all false positives. But before > even trying to solve that issue, all true positives must be fixed. Hence this > PR. src/java.se/share/data/jdwp/jdwp.spec line 47: > 45: "Returns reference types for all the classes loaded by the target > VM " > 46: "which match the given signature. " > 47: "Multiple reference types will be returned if two or more class " This file's name scares me. The fact that there's no "serviceability" label on this PR adds to this feeling. I would ask around if I were you. - PR: https://git.openjdk.java.net/jdk/pull/8364
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces
On Tue, 26 Apr 2022 22:24:26 GMT, Joe Darcy wrote: > To enable more complete doclint checking (courtesy @jonathan-gibbons), please > review this PR to add type-level @param tags where they are missing. > > To the maintainers of java.util.concurrent, those changes could be separated > out in another bug if that would ease maintenance of that code. > > Making these library fixes is a blocker for correcting and expanding the > doclint checks (JDK-8285496). > > I'll update copyright years before pushing. I note that many years ago you filed JDK-6327933, which I'm currently looking at. If implemented, many of the issues from this PR will be addressed automatically, including those in `java.util.concurrent`. - PR: https://git.openjdk.java.net/jdk/pull/8410
Integrated: 8284922: Fix some doc-comment issues on methods with package access in JDK API
On Fri, 15 Apr 2022 19:34:33 GMT, Pavel Rappo wrote: > People rarely include JDK elements with package access in a javadoc run. That > is why bugs in those elements' doc comments tend to remain unnoticed. > > There are many more bugs in the doc comments of the JDK elements with the > package access than are addressed by this PR; I only included the simplest > ones. This pull request has now been integrated. Changeset: d3d71ea2 Author:Pavel Rappo URL: https://git.openjdk.java.net/jdk/commit/d3d71ea289b7525d3f5c5057d995776be9a0796a Stats: 6 lines in 3 files changed: 1 ins; 0 del; 5 mod 8284922: Fix some doc-comment issues on methods with package access in JDK API Reviewed-by: darcy, iris, bpb - PR: https://git.openjdk.java.net/jdk/pull/8267
RFR: 8284922: Fix some doc-comment issues on methods with package access in JDK API
People rarely include JDK elements with package access in a javadoc run. That is why bugs in those elements' doc comments tend to remain unnoticed. There are many more bugs in the doc comments of the JDK elements with the package access than are addressed by this PR; I only included the simplest ones. - Commit messages: - Update copyright years - Initial commit Changes: https://git.openjdk.java.net/jdk/pull/8267/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8267=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284922 Stats: 6 lines in 3 files changed: 1 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/8267.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8267/head:pull/8267 PR: https://git.openjdk.java.net/jdk/pull/8267
Re: RFR: 8284893: Fix typos in java.base
On Fri, 15 Apr 2022 11:25:09 GMT, Pavel Rappo wrote: >> I ran `codespell` on the `src/java.base` directory, and accepted those >> changes where it indeed discovered real typos. >> >> (Due to false positives this can unfortunately not be run automatically) >> >> The majority of fixes are in comments. A handful is in strings, one in a >> local variable name, and a couple in parameter declarations. >> >> Annoyingly, there are several instances of "childs" (instead of "children") >> in the source code, but they were not local and I dared not change them. >> Someone braver than me might take a stab at it, perhaps.. > > src/java.base/share/legal/icu.md line 310: > >> 308: # list of conditions and the following disclaimer. Redistributions in >> binary >> 309: # form must reproduce the above copyright notice, this list of >> conditions and >> 310: # the following disclaimer in the documentation and/or the materials > > I think it's a mistype of "other", not "the"; look at the similar text below. Should be addressed on the ICU side. - PR: https://git.openjdk.java.net/jdk/pull/8250
Re: RFR: 8284893: Fix typos in java.base
On Thu, 14 Apr 2022 19:07:09 GMT, Magnus Ihse Bursie wrote: > I ran `codespell` on the `src/java.base` directory, and accepted those > changes where it indeed discovered real typos. > > (Due to false positives this can unfortunately not be run automatically) > > The majority of fixes are in comments. A handful is in strings, one in a > local variable name, and a couple in parameter declarations. > > Annoyingly, there are several instances of "childs" (instead of "children") > in the source code, but they were not local and I dared not change them. > Someone braver than me might take a stab at it, perhaps.. src/java.base/share/legal/icu.md line 310: > 308: # list of conditions and the following disclaimer. Redistributions in > binary > 309: # form must reproduce the above copyright notice, this list of > conditions and > 310: # the following disclaimer in the documentation and/or the materials I think it's a mistype of "other", not "the"; look at the similar text below. src/java.base/share/native/libzip/zlib/ChangeLog line 99: > 97: - Simplify contrib/vstudio/vc10 with 'd' suffix > 98: - Add TOP support to win32/Makefile.msc > 99: - Support i686 and amd64 assembler builds in CMakeLists.txt Similarly to Naoto's comment on ICU: shouldn't we leave this as is? If anybody cares enough about this typo, they could file a bug against zlib directly. src/java.base/share/native/libzip/zlib/README line 67: > 65: when compiled with cc. > 66: > 67: - On Digital Unix 4.0D (formerly OSF/1) on AlphaServer, the cc option > -std1 is Same as above. - PR: https://git.openjdk.java.net/jdk/pull/8250
Re: RFR: JDK-8280492: Address remaining doclint issues in JDK build
On Sat, 22 Jan 2022 21:09:03 GMT, Joe Darcy wrote: > Use presumed syntax that will be introduced by JDK-8280488. Is that a wrong bug? If you are talking about module-prefix syntax for links, then it was introduced in JDK 15; JDK-8164408: Add module support for @see, @link and @linkplain javadoc tags. - PR: https://git.openjdk.java.net/jdk/pull/7189
Re: RFR: JDK-8280492: Address remaining doclint issues in JDK build
On Mon, 24 Jan 2022 11:00:37 GMT, Daniel Fuchs wrote:
> LGTM. I hope in the future IDEs will pick that rule up and offer some help
> when writing `{@link }` `@see`...
They will do it quicker, if you create new or support existing bugs in their
bug trackers.
-
PR: https://git.openjdk.java.net/jdk/pull/7189
Integrated: 8279918: Fix various doc typos
On Thu, 13 Jan 2022 10:30:07 GMT, Pavel Rappo wrote: > - Most of the typos are of a trivial kind: missing whitespace. > - If any of the typos should be fixed in the upstream projects instead, > please say so; I will drop those typos from the patch. > - As I understand it, ` ` in ImageInputStream and DataInput is an irrelevant > formatting artefact and thus can be removed. > - `` is an apostrophe, which does not require to be encoded. This pull request has now been integrated. Changeset: f1805309 Author:Pavel Rappo URL: https://git.openjdk.java.net/jdk/commit/f1805309352a22119ae2edf8bfbb596f00936224 Stats: 88 lines in 35 files changed: 0 ins; 0 del; 88 mod 8279918: Fix various doc typos Reviewed-by: kevinw, lancea, mullan, sspitsyn, naoto, jlahoda, azvegint, egahlin, jjg - PR: https://git.openjdk.java.net/jdk/pull/7063
Re: RFR: 8279918: Fix various doc typos [v2]
> - Most of the typos are of a trivial kind: missing whitespace. > - If any of the typos should be fixed in the upstream projects instead, > please say so; I will drop those typos from the patch. > - As I understand it, ` ` in ImageInputStream and DataInput is an irrelevant > formatting artefact and thus can be removed. > - `` is an apostrophe, which does not require to be encoded. Pavel Rappo has updated the pull request incrementally with one additional commit since the last revision: Additional typos - Changes: - all: https://git.openjdk.java.net/jdk/pull/7063/files - new: https://git.openjdk.java.net/jdk/pull/7063/files/fe81eeca..b256a13f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7063=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7063=00-01 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/7063.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7063/head:pull/7063 PR: https://git.openjdk.java.net/jdk/pull/7063
Re: RFR: 8279918: Fix various doc typos
On Thu, 13 Jan 2022 11:38:13 GMT, Lance Andersen wrote: > Yes an "or" is probably worthwhile to add. I would prefer to leave it for the follow-up cleanup if only because adding "or" here would make it inconsistent with other `@throws SQLException` descriptions in that file and perhaps elsewhere in java.sql: * java/sql/Statement.java:59 * java/sql/Statement.java:85 * java/sql/Statement.java:346 * java/sql/Statement.java:524 * java/sql/Statement.java:745 * java/sql/Statement.java:814 * java/sql/Statement.java:860 * java/sql/Statement.java:913 * java/sql/Statement.java:962 * java/sql/Statement.java:1225 * java/sql/Statement.java:1269 * java/sql/Statement.java:1314 - PR: https://git.openjdk.java.net/jdk/pull/7063
Re: RFR: 8279918: Fix various doc typos
On Thu, 13 Jan 2022 11:42:48 GMT, Lance Andersen wrote: > The above is a bit confusing as it reads(same in ImageInputStream.java). I > think that that can be looked at later. Just to clarify: you are okay with removing the ` ` entity, right? As for ImageInputStream, some doc comments should probably use `@inheritDoc`. But this is a much bigger clean-up. - PR: https://git.openjdk.java.net/jdk/pull/7063
Re: RFR: 8279918: Fix various doc typos
On Thu, 13 Jan 2022 11:18:42 GMT, Kevin Walls wrote:
>> - Most of the typos are of a trivial kind: missing whitespace.
>> - If any of the typos should be fixed in the upstream projects instead,
>> please say so; I will drop those typos from the patch.
>> - As I understand it, ` ` in ImageInputStream and DataInput is an irrelevant
>> formatting artefact and thus can be removed.
>> - `` is an apostrophe, which does not require to be encoded.
>
> src/java.sql/share/classes/java/sql/Statement.java line 784:
>
>> 782: * statement returns a {@code ResultSet} object, the second argument
>> 783: * supplied to this method is not an
>> 784: * {@code int} array whose elements are valid column indexes, the
>> method is called on a
>
> Should it be "or the method is called on...", i.e. add the "or", otherwise
> it's a list of problems but we don't know if they are all required, or are
> alternatives. It probably does not mean that all these have to be true to
> throw the exception, but it doesn't say that. We are nitpicking of course.
You are right in that this `@throws` description reads a bit weird in its
current form. That said, I wouldn't touch it in this PR for two reasons.
Firstly, this wording seems to be consistent with other locations in that file.
Secondly, this is a spec territory.
-
PR: https://git.openjdk.java.net/jdk/pull/7063
Re: RFR: 8279918: Fix various doc typos
On Thu, 13 Jan 2022 11:00:55 GMT, Kevin Walls wrote:
>> - Most of the typos are of a trivial kind: missing whitespace.
>> - If any of the typos should be fixed in the upstream projects instead,
>> please say so; I will drop those typos from the patch.
>> - As I understand it, in ImageInputStream and DataInput is an irrelevant
>> formatting artefact and thus can be removed.
>> - is an apostrophe, which does not require to be encoded.
>
> src/java.sql/share/classes/java/sql/BatchUpdateException.java line 58:
>
>> 56: * A JDBC driver implementation should use
>> 57: * the constructor {@code BatchUpdateException(String reason, String
>> SQLState,
>> 58: * int vendorCode, long []updateCounts, Throwable cause) } instead of
>
> While we are here, is it more normal to say "long[] updateCounts", not
> separating the [] from the type.
> Similarly at line 81, 118, 151, 247, 277, 318, 354.
I thought about it too, but then noticed how the position of `[]` mimics that
of the respective method signatures in code.
-
PR: https://git.openjdk.java.net/jdk/pull/7063
Re: RFR: 8279918: Fix various doc typos
On Thu, 13 Jan 2022 10:46:11 GMT, Kevin Walls wrote: >> - Most of the typos are of a trivial kind: missing whitespace. >> - If any of the typos should be fixed in the upstream projects instead, >> please say so; I will drop those typos from the patch. >> - As I understand it, in ImageInputStream and DataInput is an irrelevant >> formatting artefact and thus can be removed. >> - is an apostrophe, which does not require to be encoded. > > src/java.base/share/classes/sun/text/RuleBasedBreakIterator.java line 73: > >> 71: * will be transparent to the BreakIterator. A sequence of >> characters will break the >> 72: * same way it would if any ignore characters it contains are taken >> out. Break >> 73: * positions never occur before ignore characters. > > "before ignored characters" I believe it's the name of a concept, so I will leave it as is: > There is one special substitution. If the description defines a substitution > called "", the expression must be a [] expression, and the expression > defines a set of characters (the "ignore characters") that will be > transparent to the BreakIterator. - PR: https://git.openjdk.java.net/jdk/pull/7063
RFR: 8279918: Fix various doc typos
- Most of the typos are of a trivial kind: missing whitespace. - If any of the typos should be fixed in the upstream projects instead, please say so; I will drop those typos from the patch. - As I understand it, in ImageInputStream and DataInput is an irrelevant formatting artefact and thus can be removed. - is an apostrophe, which does not require to be encoded. - Commit messages: - Initial commit Changes: https://git.openjdk.java.net/jdk/pull/7063/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7063=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8279918 Stats: 85 lines in 34 files changed: 0 ins; 0 del; 85 mod Patch: https://git.openjdk.java.net/jdk/pull/7063.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7063/head:pull/7063 PR: https://git.openjdk.java.net/jdk/pull/7063
Re: RFR: JDK-8276674 : Malformed Javadoc inline tags in JDK source in java/util/stream
On Thu, 18 Nov 2021 03:22:50 GMT, Tim Prinzing wrote: > JDK-8276674 : Malformed Javadoc inline tags in JDK source in java/util/stream Given that this PR affects files that aren't rooted under java/util/stream, I would either rename the PR or exclude the unrelated files. If this PR is going to sit there for some more time, I would prefer the latter. This is because the PR has already conflicted with a few other issues related to malformed javadoc tags (e.g. JDK-8276683 and JDK-8276684). So either rename and integrate soon, or exclude the unrelated files. - PR: https://git.openjdk.java.net/jdk/pull/6443
Re: RFR: JDK-8276674 : Malformed Javadoc inline tags in JDK source in java/util/stream
On Thu, 25 Nov 2021 00:31:46 GMT, Pavel Rappo wrote:
>> JDK-8276674 : Malformed Javadoc inline tags in JDK source in java/util/stream
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/util/RawDiagnosticFormatter.java
> line 63:
>
>> 61: * Helper class to generate stable positions for AST nodes occurring
>> in diagnostic arguments.
>> 62: * If the AST node appears in the same line number as the main
>> diagnostic, the line is information is omitted.
>> 63: * Otherwise both line and column information is included, using the
>> format {@code line:col}".
>
> Not a showstopper by any means. But while you are at it, maybe you could also
> fix this?
>
> Suggestion:
>
> * If the AST node appears in the same line number as the main
> diagnostic, the line information is omitted.
> * Otherwise both line and column information is included, using the
> format {@code line:col}.
This part was recently integrated in https://github.com/openjdk/jdk/pull/6602.
I assume there will be a trivial merge conflict to resolve.
-
PR: https://git.openjdk.java.net/jdk/pull/6443
Re: RFR: JDK-8276674 : Malformed Javadoc inline tags in JDK source in java/util/stream
On Thu, 18 Nov 2021 03:22:50 GMT, Tim Prinzing wrote:
> JDK-8276674 : Malformed Javadoc inline tags in JDK source in java/util/stream
Looks good.
src/jdk.compiler/share/classes/com/sun/tools/javac/util/RawDiagnosticFormatter.java
line 63:
> 61: * Helper class to generate stable positions for AST nodes occurring
> in diagnostic arguments.
> 62: * If the AST node appears in the same line number as the main
> diagnostic, the line is information is omitted.
> 63: * Otherwise both line and column information is included, using the
> format {@code line:col}".
Not a showstopper by any means. But while you are at it, maybe you could also
fix this?
Suggestion:
* If the AST node appears in the same line number as the main diagnostic,
the line information is omitted.
* Otherwise both line and column information is included, using the format
{@code line:col}.
-
Marked as reviewed by prappo (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/6443
Re: RFR: JDK-8276674 : Malformed Javadoc inline tags in JDK source in java/util/stream
On Thu, 18 Nov 2021 19:18:24 GMT, Jonathan Gibbons wrote: > Remarkable to have not been noticed for so long! Remarkable, but not too surprising: the PR consists of source files and tests that are not part of the API Documentation. - PR: https://git.openjdk.java.net/jdk/pull/6443
Re: RFR: 8277535: Remove redundant Stream.distinct()/sorted() steps
On Mon, 27 Sep 2021 11:20:53 GMT, Andrey Turbanov wrote: > 1. Stream.distinct() is redundant before toSet() collector. Duplicates will > be collapsed by Collector. > 2. Stream.sorted() is redundant before toMap() collector. Keys will be > shuffled by Collector (it's a HashMap in current implementation) Looks good. Please update the copyright years before integrating. Note that in general, removing distinct() immediately before Collectors.toSet() might change the behavior. If operated on an ordered stream, distinct() is guaranteed to yield a stable order. - Marked as reviewed by prappo (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5714
Re: RFR: JDK-8277095 : Empty streams create too many objects
On Fri, 5 Nov 2021 12:53:46 GMT, kabutz wrote:
> This is a draft proposal for how we could improve stream performance for the
> case where the streams are empty. Empty collections are common-place. If we
> iterate over them with an Iterator, we would have to create one small
> Iterator object (which could often be eliminated) and if it is empty we are
> done. However, with Streams we first have to build up the entire pipeline,
> until we realize that there is no work to do. With this example, we change
> Collection#stream() to first check if the collection is empty, and if it is,
> we simply return an EmptyStream. We also have EmptyIntStream, EmptyLongStream
> and EmptyDoubleStream. We have taken great care for these to have the same
> characteristics and behaviour as the streams returned by Stream.empty(),
> IntStream.empty(), etc.
>
> Some of the JDK tests fail with this, due to ClassCastExceptions (our
> EmptyStream is not an AbstractPipeline) and AssertionError, since we can call
> some methods repeatedly on the stream without it failing. On the plus side,
> creating a complex stream on an empty stream gives us upwards of 50x increase
> in performance due to a much smaller object allocation rate. This PR includes
> the code for the change, unit tests and also a JMH benchmark to demonstrate
> the improvement.
Streams are closeable, and a terminal operation may be invoked on a given
stream only once. Thus, shouldn't the third line in both of the examples below
throw `IllegalStateException`?
Stream empty = Stream.empty();
System.out.println(empty.count());
System.out.println(empty.count());
Stream empty = Stream.empty();
empty.close();
System.out.println(empty.count());
I don't think that we can remove all the state from an empty stream, but we can
likely make it smaller.
src/java.base/share/classes/java/util/Collection.java line 743:
> 741: */
> 742: default Stream stream() {
> 743: if (isEmpty()) return Stream.empty();
The net effect of this change might depend on your workload. If you call
stream() on empty collections that have cheap isEmpty(), this change will
likely improve performance and reduce waste. However, this same change might do
the opposite if some of your collections aren't empty or have costly isEmpty().
It would be good to have benchmarks for different workloads.
-
PR: https://git.openjdk.java.net/jdk/pull/6275
Integrated: 8277048: Tiny improvements to the specification text for java.util.Properties.load
On Fri, 12 Nov 2021 12:25:20 GMT, Pavel Rappo wrote: > Please review this simple two-hunk fix to the documentation comment of > java.util.Properties#load(java.io.Reader). The first hunk (line/lines) is a > suggestion. I believe it reads better since the plurality is more idiomatic; > native English speakers should correct me if I'm wrong. The second hunk picks > up what was overlooked in JDK-8274075 > (https://git.openjdk.java.net/jdk/pull/5610). This pull request has now been integrated. Changeset: fdcd16a3 Author:Pavel Rappo URL: https://git.openjdk.java.net/jdk/commit/fdcd16a38fb9a14a819d68682f9666ebfe7285db Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod 8277048: Tiny improvements to the specification text for java.util.Properties.load Reviewed-by: rriggs, iris, naoto - PR: https://git.openjdk.java.net/jdk/pull/6367
RFR: 8277048: Tiny improvements to the specification text for java.util.Properties.load
Please review this simple two-hunk fix to the documentation comment of java.util.Properties#load(java.io.Reader). The first hunk (line/lines) is a suggestion. I believe it reads better since the plurality is more idiomatic; native English speakers should correct me if I'm wrong. The second hunk picks up what was overlooked in JDK-8274075 (https://git.openjdk.java.net/jdk/pull/5610). - Commit messages: - Initial commit Changes: https://git.openjdk.java.net/jdk/pull/6367/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6367=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8277048 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/6367.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6367/head:pull/6367 PR: https://git.openjdk.java.net/jdk/pull/6367
Re: RFR: 8276186: Require getAvailableLocales() methods to include Locale.ROOT
On Thu, 4 Nov 2021 16:07:01 GMT, Naoto Sato wrote:
> This fix is to require to include `Locale.ROOT` in the returned arrays/set
> from `getAvailableLocales()` methods in various locale-sensitive classes. The
> implementation has been including `Locale.ROOT` since its inception, it is
> simply a doc clarification (+ a test case verifying it). Corresponding CSR
> has also been drafted: https://bugs.openjdk.java.net/browse/JDK-8276249
The change to the existing source looks good, and the new test looks beautiful.
Thanks for doing this.
src/java.base/share/classes/java/time/format/DecimalStyle.java line 118:
> 116: * Lists all the locales that are supported.
> 117: *
> 118: * At a minimum, the returned Set must contain a {@code Locale}
> instance equal to
A nit, really. Consider applying either of these suggestions:
Suggestion:
* At a minimum, the returned {@code Set} must contain a {@code Locale}
instance equal to
Suggestion:
* At a minimum, the returned set must contain a {@code Locale} instance
equal to
-
Marked as reviewed by prappo (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/6258
Integrated: 8276338: Minor improve of wording for String.to(Lower|Upper)Case
On Wed, 3 Nov 2021 11:58:42 GMT, Pavel Rappo wrote: > This PR fixes two sentences which conflate a string with its length, and also > makes the "equivalency wording" consistent. > > There are many ways to fix "the resulting String may be a different length > than the original String". The proposed way might be one of the most > lightweight ways to do that. This pull request has now been integrated. Changeset: bf4ddf9c Author:Pavel Rappo URL: https://git.openjdk.java.net/jdk/commit/bf4ddf9cb7408db321e7fd73600a5a9a85ca49eb Stats: 8 lines in 1 file changed: 0 ins; 1 del; 7 mod 8276338: Minor improve of wording for String.to(Lower|Upper)Case Reviewed-by: rriggs, naoto - PR: https://git.openjdk.java.net/jdk/pull/6232
Re: RFR: 8276338: Minor improve of wording for String.to(Lower|Upper)Case [v2]
> This PR fixes two sentences which conflate a string with its length, and also > makes the "equivalency wording" consistent. > > There are many ways to fix "the resulting String may be a different length > than the original String". The proposed way might be one of the most > lightweight ways to do that. Pavel Rappo has updated the pull request incrementally with one additional commit since the last revision: Fix inconsistent formatting - Removes stray whitespace character *inside* a sentence. - Makes sentences that introduce tables more similar. - Changes: - all: https://git.openjdk.java.net/jdk/pull/6232/files - new: https://git.openjdk.java.net/jdk/pull/6232/files/897ac5bb..625ae1aa Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6232=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6232=00-01 Stats: 3 lines in 1 file changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/6232.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6232/head:pull/6232 PR: https://git.openjdk.java.net/jdk/pull/6232
Re: RFR: 8276629: Use blessed modifier order in core-libs code
On Thu, 4 Nov 2021 11:09:42 GMT, Magnus Ihse Bursie wrote: > I ran bin/blessed-modifier-order.sh on source owned by core-libs. This > scripts verifies that modifiers are in the "blessed" order, and fixes it > otherwise. I have manually checked the changes made by the script to make > sure they are sound. I can see that this PR changes java.naming. Another bug to change java.naming, JDK-8276552, was filed yesterday. Please check with its reporter and coordinate this effort if necessary. - PR: https://git.openjdk.java.net/jdk/pull/6250
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 16:30:56 GMT, Pavel Rappo wrote:
> This PR follows up one of the recent PRs, where I used a non-canonical
> modifier order. Since the problem was noticed [^1], why not to address it en
> masse?
>
> As far as I remember, the first mass-canonicalization of modifiers took place
> in JDK-8136583 in 2015 [^2]. That change affected 1780 lines spanning 453
> files. Since then modifiers have become a bit loose, and it makes sense to
> re-bless (using the JDK-8136583 terminology) them.
>
> This change was produced by running the below command followed by updating
> the copyright years on the affected files where necessary:
>
> $ sh ./bin/blessed-modifier-order.sh src/java.base
>
> The resulting change is much smaller than that of 2015: 39 lines spanning 21
> files.
>
> [^1]:
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/082987.html
> (or https://github.com/openjdk/jdk/pull/6191#pullrequestreview-79465)
> [^2]:
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035217.html
> _Mailing list message from [David Holmes](mailto:david.hol...@oracle.com) on
> [core-libs-dev](mailto:core-libs-...@mail.openjdk.java.net):_
>
> On 3/11/2021 9:26 pm, Pavel Rappo wrote:
>
> > On Tue, 2 Nov 2021 20:34:44 GMT, Martin Buchholz
> > wrote:
> > > > > Pragmatically, fix the script to ignore those keywords on comment
> > > > > lines. Learn Perl, its just a regular expression pattern match and
> > > > > replace expression.
> > > >
> > > >
> > > > I understand in principle how to modify that script to ignore doc
> > > > comments. The thing I was referring to when said "btw, how would we do
> > > > that?" was this: not all comment lines are prose. Some of those lines
> > > > belong to snippets of code, which I guess you would also like to be
> > > > properly formatted.
> > > > > But having seen several reviewers be unmoved by the difference, the
> > > > > real pragmatic view is to ignore the English.
> > > >
> > > >
> > > > I'm sorry you feel that way. Would it be okay if I made it clear that
> > > > those two words are not English adjectives but are special symbols that
> > > > happen to use Latin script and originate from the English words they
> > > > resemble? If so, I could enclose each of them in `{@code ... }`. If
> > > > not, I could drop that particular change from this PR.
> > >
> > >
> > > The blessed-modifier-order.sh script intentionally modifies comments,
> > > with the hope of finding code snippets (it did!)
> > > Probably I manually deleted the change to Object.java back in 2015, to
> > > avoid the sort of controversy we're seeing now.
> > > I don't have a strong feeling either way on changing that file.
> > > I agree with @pavelrappo that script-generated changes should not be
> > > mixed with manual changes.
> > > I would also not update copyright years for such changes.
> > > It's a feature of blessed-modifier-order.sh that all existing formatting
> > > is perfectly preserved.
> >
> >
> > One more thing. Please have a look at this other line in the same file;
> > this line was there before the change
> > https://github.com/openjdk/jdk/blob/465d350d0b3cac277a58b9f8ece196c1cde68e80/src/java.base/share/classes/java/lang/Object.java#L49
> > So before the change, the file was somewhat inconsistent. The change made
> > it consistent. **If one is going to ever revert that controversial part of
> > the change, please update both lines so that the file remains consistent.**
>
> Line 281 is (was!) consistent with line 277 because it is distinguishing a
> synchronized "static method" from a synchronized "instance method". There was
> no need to make any change to line 281 because of the blessed-order of
> modifiers as defined for method declarations! This text is just prose. Now
> for consistency you should change line 277 to refer to a "non-static
> synchronized method" (as "instance synchronized method" would be truly awful).
Thanks, David. You've provided a clear and convincing argument, and I can see
the inconsistency I introduced. I can revert that particular piece back if you
think that it would be appropriate.
That said, this line will have to be filtered out every time the script is run.
I could probably provide a more involved script that harnesses the power of AST
(com.sun.source.doctree) to try to filter out prose, but it would be
RFR: 8276338: Minor improve of wording for String.to(Lower|Upper)Case
This PR fixes two sentences which conflate a string with its length, and also makes the "equivalency wording" consistent. There are many ways to fix "the resulting String may be a different length than the original String". The proposed way might be one of the most lightweight ways to do that. - Commit messages: - Initial commit Changes: https://git.openjdk.java.net/jdk/pull/6232/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6232=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276338 Stats: 5 lines in 1 file changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/6232.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6232/head:pull/6232 PR: https://git.openjdk.java.net/jdk/pull/6232
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 20:34:44 GMT, Martin Buchholz wrote:
>>> Pragmatically, fix the script to ignore those keywords on comment lines.
>>> Learn Perl, its just a regular expression pattern match and replace
>>> expression.
>>
>> I understand in principle how to modify that script to ignore doc comments.
>> The thing I was referring to when said "btw, how would we do that?" was
>> this: not all comment lines are prose. Some of those lines belong to
>> snippets of code, which I guess you would also like to be properly formatted.
>>
>>> But having seen several reviewers be unmoved by the difference, the real
>>> pragmatic view is to ignore the English.
>>
>> I'm sorry you feel that way. Would it be okay if I made it clear that those
>> two words are not English adjectives but are special symbols that happen to
>> use Latin script and originate from the English words they resemble? If so,
>> I could enclose each of them in `{@code ... }`. If not, I could drop that
>> particular change from this PR.
>
> The blessed-modifier-order.sh script intentionally modifies comments, with
> the hope of finding code snippets (it did!)
>
> Probably I manually deleted the change to Object.java back in 2015, to avoid
> the sort of controversy we're seeing now.
> I don't have a strong feeling either way on changing that file.
>
> I agree with @pavelrappo that script-generated changes should not be mixed
> with manual changes.
> I would also not update copyright years for such changes.
>
> It's a feature of blessed-modifier-order.sh that all existing formatting is
> perfectly preserved.
One more thing. Please have a look at this other line in the same file; this
line was there before the change
https://github.com/openjdk/jdk/blob/465d350d0b3cac277a58b9f8ece196c1cde68e80/src/java.base/share/classes/java/lang/Object.java#L49
So before the change, the file was somewhat inconsistent. The change made it
consistent. **If one is going to ever revert that controversial part of the
change, please update both lines so that the file remains consistent.**
-
PR: https://git.openjdk.java.net/jdk/pull/6213
Integrated: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 16:30:56 GMT, Pavel Rappo wrote: > This PR follows up one of the recent PRs, where I used a non-canonical > modifier order. Since the problem was noticed [^1], why not to address it en > masse? > > As far as I remember, the first mass-canonicalization of modifiers took place > in JDK-8136583 in 2015 [^2]. That change affected 1780 lines spanning 453 > files. Since then modifiers have become a bit loose, and it makes sense to > re-bless (using the JDK-8136583 terminology) them. > > This change was produced by running the below command followed by updating > the copyright years on the affected files where necessary: > > $ sh ./bin/blessed-modifier-order.sh src/java.base > > The resulting change is much smaller than that of 2015: 39 lines spanning 21 > files. > > [^1]: > https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/082987.html > (or https://github.com/openjdk/jdk/pull/6191#pullrequestreview-79465) > [^2]: > http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035217.html This pull request has now been integrated. Changeset: 61506336 Author:Pavel Rappo URL: https://git.openjdk.java.net/jdk/commit/615063364ab6bdd3fa83401745e05b45e13eacdb Stats: 39 lines in 21 files changed: 0 ins; 0 del; 39 mod 8276348: Use blessed modifier order in java.base Reviewed-by: dfuchs, darcy, iris, rriggs, martin - PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 19:22:15 GMT, Pavel Rappo wrote:
>> src/java.base/share/classes/java/lang/invoke/CallSite.java line 88:
>>
>>> 86: */
>>> 87: public
>>> 88: abstract class CallSite {
>>
>> I think it's better to move all modifiers to the single line.
>
> This is a survivorship bias. This example jumps out at you, because it
> happens to use missorted modifiers. I'm pretty sure this is not an
> aberration, but a style. If you want to change that style, you should create
> a respective JBS issue.
I've grepped the code and can now see that a list of modifiers broken by
newlines which cannot be explained by line-width concerns is indeed an
irregularity. Not only in java.base but also in other modules. Although there
aren't many of such cases, I would prefer them to be addressed in a separate
cleanup, which you are welcome to pursue.
-
PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 19:15:26 GMT, Andrey Turbanov wrote:
>> This PR follows up one of the recent PRs, where I used a non-canonical
>> modifier order. Since the problem was noticed [^1], why not to address it at
>> mass?
>>
>> As far as I remember, the first mass-canonicalization of modifiers took
>> place in JDK-8136583 in 2015 [^2]. That change affected 1780 lines spanning
>> 453 files. Since then modifiers have become a bit loose, and it makes sense
>> to re-bless (using the JDK-8136583 terminology) them.
>>
>> This change was produced by running the below command followed by updating
>> the copyright years on the affected files where necessary:
>>
>> $ sh ./bin/blessed-modifier-order.sh src/java.base
>>
>> The resulting change is much smaller than that of 2015: 39 lines spanning 21
>> files.
>>
>> [^1]:
>> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/082987.html
>> (or https://github.com/openjdk/jdk/pull/6191#pullrequestreview-79465)
>> [^2]:
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035217.html
>
> src/java.base/share/classes/java/lang/invoke/CallSite.java line 88:
>
>> 86: */
>> 87: public
>> 88: abstract class CallSite {
>
> I think it's better to move all modifiers to the single line.
This is a survivorship bias. This example jumps out at you, because it happens
to use missorted modifiers. I'm pretty sure this is not an aberration, but a
style. If you want to change that style, you should create a respective JBS
issue.
-
PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 18:48:20 GMT, Roger Riggs wrote:
> Pragmatically, fix the script to ignore those keywords on comment lines.
> Learn Perl, its just a regular expression pattern match and replace
> expression.
I understand in principle how to modify that script to ignore doc comments. The
thing I was referring to when said "btw, how would we do that?" was this: not
all comment lines are prose. Some of those lines belong to snippets of code,
which I guess you would also like to be properly formatted.
> But having seen several reviewers be unmoved by the difference, the real
> pragmatic view is to ignore the English.
I'm sorry you feel that way. Would it be okay if I made it clear that those two
words are not English adjectives but are special symbols that happen to use
Latin script and originate from the English words they resemble? If so, I could
enclose each of them in `{@code ... }`. If not, I could drop that particular
change from this PR.
-
PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 17:45:07 GMT, Pavel Rappo wrote: >>> In comments, I think the 'synchronized static 'reads better, the >>> conventional order is for the code. >> >> I think Roger is right and maybe the change to the javadoc could be dropped >> from this patch. > > It's tough when a natural language clashes with a programming language. I > appreciate that this particular clash might cause discomfort to native > English speakers. (This reminds me of that _DOSASCOMP_ mnemonic for adjective > order.) That said, consider the following pragmatic aspect. Unless we change > the script not to process prose in comments (btw, how would we do that?), > every single time we run that script, that particular line in Object.java > will need to be tracked and excluded. Here's a bit of archaeology. I found the original JDK-8136583 patch, which has moved from where it was in the RFR to https://cr.openjdk.java.net/~martin/webrevs/jdk9/blessed-modifier-order/. That patch doesn't change Object.java. The RFR thread mentions neither that javadoc change nor any javadoc change for that matter. So either the script was different, or Martin filtered that line (from Object.java) out before creating the webrev. Now, in his followup thread on core-libs-dev, http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035273.html, Martin specifically pointed out javadoc changes and said that they seem fine to him. - PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 17:39:17 GMT, Alan Bateman wrote:
>> src/java.base/share/classes/java/lang/Object.java line 282:
>>
>>> 280: * For objects of type {@code Class,} by executing a
>>> 281: * static synchronized method of that class.
>>> 282: *
>>
>> In comments, I think the 'synchronized static 'reads better, the
>> conventional order is for the code.
>
>> In comments, I think the 'synchronized static 'reads better, the
>> conventional order is for the code.
>
> I think Roger is right and maybe the change to the javadoc could be dropped
> from this patch.
It's tough when a natural language clashes with a programming language. I
appreciate that this particular clash might cause discomfort to native English
speakers. (This reminds me of that _DOSASCOMP_ mnemonic for adjective order.)
That said, consider the following pragmatic aspect. Unless we change the script
not to process prose in comments (btw, how would we do that?), every single
time we run that script, that particular line in Object.java will need to be
tracked and excluded.
-
PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 16:30:56 GMT, Pavel Rappo wrote: > This PR follows up one of the recent PRs, where I used a non-canonical > modifier order. Since the problem was noticed [^1], why not to address it at > mass? > > As far as I remember, the first mass-canonicalization of modifiers took place > in JDK-8136583 in 2015 [^2]. That change affected 1780 lines spanning 453 > files. Since then modifiers have become a bit lose, and it makes sense to > re-bless (using the JDK-8136583 terminology) them. > > This change was produced by running the below command followed by updating > the copyright years on the affected files where necessary: > > $ sh ./bin/blessed-modifier-order.sh src/java.base > > The resulting change is much smaller than that of 2015: 39 lines spanning 21 > files. > > [^1]: > https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/082987.html > (or https://github.com/openjdk/jdk/pull/6191#pullrequestreview-79465) > [^2]: > http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035217.html A colleague suggested that I should clarify that the `blessed-modifier-order.sh` script that I used is available in the JDK repo at https://github.com/openjdk/jdk/blob/01105d6985b39d4064b9066eab3612da5a401685/bin/blessed-modifier-order.sh. That script was contributed by Martin Buchholz in JDK-8136656 in 2015. - PR: https://git.openjdk.java.net/jdk/pull/6213
RFR: 8276348: Use blessed modifier order in java.base
This PR follows up one of the recent PRs, where I used a non-canonical modifier order. Since the problem was noticed [^1], why not to address it at mass? As far as I remember, the first mass-canonicalization of modifiers took place in JDK-8136583 in 2015 [^2]. That change affected 1780 lines spanning 453 files. Since then modifiers have become a bit lose, and it makes sense to re-bless (using the JDK-8136583 terminology) them. This change was produced by running the below command followed by updating the copyright years on the affected files where necessary: $ sh ./bin/blessed-modifier-order.sh src/java.base The resulting change is much smaller than that of 2015: 39 lines spanning 21 files. [^1]: https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/082987.html (or https://github.com/openjdk/jdk/pull/6191#pullrequestreview-79465) [^2]: http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035217.html - Commit messages: - Initial commit Changes: https://git.openjdk.java.net/jdk/pull/6213/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6213=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276348 Stats: 39 lines in 21 files changed: 0 ins; 0 del; 39 mod Patch: https://git.openjdk.java.net/jdk/pull/6213.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6213/head:pull/6213 PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8276234: Trivially clean up locale-related code [v2]
On Mon, 1 Nov 2021 16:51:36 GMT, Pavel Rappo wrote: >> Please review this PR. A comprehensive test job has been scheduled; I'll >> notify this thread once that job has completed. > > Pavel Rappo has updated the pull request incrementally with one additional > commit since the last revision: > > Use the blessed modifiers order The test job that has been scheduled previously has completed successfully. - PR: https://git.openjdk.java.net/jdk/pull/6191
Integrated: 8276234: Trivially clean up locale-related code
On Mon, 1 Nov 2021 15:04:16 GMT, Pavel Rappo wrote: > Please review this PR. A comprehensive test job has been scheduled; I'll > notify this thread once that job has completed. This pull request has now been integrated. Changeset: 2eafa036 Author: Pavel Rappo URL: https://git.openjdk.java.net/jdk/commit/2eafa036c03d3e8f3dba8f67dd37b484874dc3d3 Stats: 13 lines in 3 files changed: 0 ins; 1 del; 12 mod 8276234: Trivially clean up locale-related code Reviewed-by: redestad, naoto, iris - PR: https://git.openjdk.java.net/jdk/pull/6191
Re: RFR: 8276234: Trivially clean up locale-related code [v2]
On Mon, 1 Nov 2021 16:25:26 GMT, Pavel Rappo wrote: >> src/java.base/share/classes/sun/util/resources/LocaleData.java line 248: >> >>> 246: private static final LocaleDataStrategy INSTANCE = new >>> LocaleDataStrategy(); >>> 247: // TODO: avoid hard-coded Locales >>> 248: private final static Set JAVA_BASE_LOCALES >> >> Canonical modifier order is `static final` > > It turns out that my IDE was configured NOT to highlight missorted modifiers. > Once I reconfigured it, I saw these cases and some other in that same file. > I'll fix them all if that's okay. > > My recollection is that there have been campaigns on using "the blessed > modifiers order". It might be that since the last such crusade the modifiers > have gone out of hand, and we might need to re-bless them :-) Fixed in 2b7e0c6. - PR: https://git.openjdk.java.net/jdk/pull/6191
Re: RFR: 8276234: Trivially clean up locale-related code [v2]
> Please review this PR. A comprehensive test job has been scheduled; I'll > notify this thread once that job has completed. Pavel Rappo has updated the pull request incrementally with one additional commit since the last revision: Use the blessed modifiers order - Changes: - all: https://git.openjdk.java.net/jdk/pull/6191/files - new: https://git.openjdk.java.net/jdk/pull/6191/files/8e2815b8..2b7e0c68 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6191=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6191=00-01 Stats: 5 lines in 1 file changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/6191.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6191/head:pull/6191 PR: https://git.openjdk.java.net/jdk/pull/6191
Re: RFR: 8276234: Trivially clean up locale-related code
On Mon, 1 Nov 2021 15:23:49 GMT, Claes Redestad wrote: >> Please review this PR. A comprehensive test job has been scheduled; I'll >> notify this thread once that job has completed. > > src/java.base/share/classes/sun/util/resources/LocaleData.java line 248: > >> 246: private static final LocaleDataStrategy INSTANCE = new >> LocaleDataStrategy(); >> 247: // TODO: avoid hard-coded Locales >> 248: private final static Set JAVA_BASE_LOCALES > > Canonical modifier order is `static final` It turns out that my IDE was configured NOT to highlight missorted modifiers. Once I reconfigured it, I saw these cases and some other in that same file. I'll fix them all if that's okay. My recollection is that there have been campaigns on using "the blessed modifiers order". It might be that since the last such crusade the modifiers have gone out of hand, and we might need to re-bless them :-) - PR: https://git.openjdk.java.net/jdk/pull/6191
Re: RFR: 8276234: Trivially clean up locale-related code
On Mon, 1 Nov 2021 15:52:51 GMT, Daniel Fuchs wrote:
>> Please review this PR. A comprehensive test job has been scheduled; I'll
>> notify this thread once that job has completed.
>
> src/java.base/share/classes/sun/util/resources/LocaleData.java line 336:
>
>> 334: public List getCandidateLocales(String baseName, Locale
>> locale) {
>> 335: // Specify only the given locale
>> 336: return List.of(locale);
>
> Is it guaranteed that `locale` is not `null` here?
This is a good question. As far I can see, the only call site checks for null
explicitly:
https://github.com/openjdk/jdk/blob/a4c46e1e4f4f2f05c8002b2af683a390fc46b424/src/java.base/share/classes/sun/util/resources/Bundles.java#L113
So the answer is yes.
-
PR: https://git.openjdk.java.net/jdk/pull/6191
RFR: 8276234: Trivially clean up locale-related code
Please review this PR. A comprehensive test job has been scheduled; I'll notify this thread once that job has completed. - Commit messages: - Fix *code* typo - Be more immutable - Fix typos Changes: https://git.openjdk.java.net/jdk/pull/6191/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6191=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276234 Stats: 10 lines in 3 files changed: 0 ins; 1 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/6191.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6191/head:pull/6191 PR: https://git.openjdk.java.net/jdk/pull/6191
Re: RFR: 8275293: A change done with JDK-8268764 mismatches the java.rmi.server.ObjID.hashCode spec
On Fri, 15 Oct 2021 07:17:52 GMT, Сергей Цыпанов wrote: > It looks like we cannot use `Long.hashCode(long)` for > `java.rmi.server.ObjID.hashCode()` due to JavaDoc: > https://docs.oracle.com/en/java/javase/17/docs/api/java.rmi/java/rmi/server/ObjID.html#hashCode(). It's important to be precise here: it's not "JavaDoc", it's the specification. - PR: https://git.openjdk.java.net/jdk/pull/5963
Re: RFR: 8275002: Remove unused AbstractStringBuilder.MAX_ARRAY_SIZE [v3]
On Mon, 11 Oct 2021 21:07:21 GMT, Andrey Turbanov wrote: >> 8275002: Remove unused AbstractStringBuilder.MAX_ARRAY_SIZE > > Andrey Turbanov has updated the pull request incrementally with one > additional commit since the last revision: > > 8275002: Remove unused AbstractStringBuilder.MAX_ARRAY_SIZE > avoid link in private javadoc Marked as reviewed by prappo (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5878
Re: RFR: 8275002: Remove unused AbstractStringBuilder.MAX_ARRAY_SIZE [v2]
On Mon, 11 Oct 2021 18:52:07 GMT, Andrey Turbanov wrote: >> 8275002: Remove unused AbstractStringBuilder.MAX_ARRAY_SIZE > > Andrey Turbanov has updated the pull request incrementally with one > additional commit since the last revision: > > 275002: Remove unused AbstractStringBuilder.MAX_ARRAY_SIZE > update javadoc of 'newCapacity' method to refer > ArraysSupport.SOFT_MAX_ARRAY_LENGTH instead I'll run tests; if they pass, I'll sponsor the change. - PR: https://git.openjdk.java.net/jdk/pull/5878
Re: RFR: 8275002: Remove unused AbstractStringBuilder.MAX_ARRAY_SIZE
On Sat, 9 Oct 2021 17:54:16 GMT, Andrey Turbanov wrote: > 8275002: Remove unused AbstractStringBuilder.MAX_ARRAY_SIZE Oops. I think we should also do something about this occurrence of MAX_ARRAY_SIZE: https://github.com/openjdk/jdk/blob/d679bd3ab8b41a14359d3bfb9763a1178d02ecb0/src/java.base/share/classes/java/lang/AbstractStringBuilder.java#L239 - PR: https://git.openjdk.java.net/jdk/pull/5878
Re: RFR: 8275002: Remove unused AbstractStringBuilder.MAX_ARRAY_SIZE
On Sat, 9 Oct 2021 17:54:16 GMT, Andrey Turbanov wrote: > 8275002: Remove unused AbstractStringBuilder.MAX_ARRAY_SIZE Marked as reviewed by prappo (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5878
Re: Will it be worthy to add some public static final empty arrays for some most used types in some place in java.base, other than create them multi times in several places?
Empty? As in arrays of length zero? There ought to be a much better solution. I'm neither an expert nor (sadly) an active follower of the Valhalla project [1]. Perhaps something there or in Frozen Arrays [2] will address the runtime cost of creating zero-length arrays much more elegantly. [1] https://openjdk.java.net/projects/valhalla/ [2] https://openjdk.java.net/jeps/8261007 -Pavel > On 9 Oct 2021, at 18:19, Xeno Amess wrote: > > I suggest putting them in Arrays btw.
Re: Unexepcted OutOfMemoryError from Arrays.deepToString
> On 9 Oct 2021, at 13:07, Pavel Rappo wrote: > > > > Separately, java.lang.AbstractStringBuilder#MAX_ARRAY_SIZE seems unused; I > wonder how that happened. I found what happened: $ git log -S "MAX_ARRAY_SIZE" -- src/java.base/share/classes/java/lang/AbstractStringBuilder.java commit 03642a01af7123298d6524a98c99a3934d35c11b Author: Jim Laskey Date: Thu Jun 11 10:08:23 2020 -0300 8230744: Several classes throw OutOfMemoryError without message Reviewed-by: psandoz, martin, bchristi, rriggs, smarks Looking at the corresponding review thread, https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-June/066874.html, the question about "the local orphan" MAX_ARRAY_SIZE was raised by Martin and subsequently addressed by Jim. The problem is, there were two of these fields. The one in PriorityBlockingQueue was spotted and deleted, the one in AbstractStringBuilder was not. Andrey, regardless of the outcome of your main question (OOM from Arrays.deepToString), would be willing to publish a PR to delete that field in AbstractStringBuilder? -Pavel > > -Pavel > >> On 9 Oct 2021, at 11:38, Andrey Turbanov wrote: >> >> Hello. >> I came across unexpected behaviour of Arrays.deepToString method. >> It throws OOM even on non-huge arrays. >> For example this code: >> >> int size = Integer.MAX_VALUE / 19; >> Integer[] integers = new Integer[size]; >> Arrays.fill(integers, 0); >> System.out.println(Arrays.deepToString(integers)); >> >> Fails with following stack trace: >> >> Exception in thread "main" java.lang.OutOfMemoryError: Requested array >> size exceeds VM limit >> at >> java.base/java.lang.AbstractStringBuilder.(AbstractStringBuilder.java:86) >> at java.base/java.lang.StringBuilder.(StringBuilder.java:112) >> at java.base/java.util.Arrays.deepToString(Arrays.java:5160) >> >> >> I believe it should be able to handle such arrays and not throw OOM >> >> >> Andrey Turbanov >
Re: Unexepcted OutOfMemoryError from Arrays.deepToString
This error has two causes. The first cause is that the VM cannot allocate arrays whose length exceeds Integer.MAX_VALUE - 8 (MAX_ARRAY_SIZE). The second cause is that Arrays.deepToString tries to pre-allocate 20 chars per string representation for each array element and maxes out at Integer.MAX_VALUE, which is above MAX_ARRAY_SIZE. One solution would be to change Arrays.deepToString to max out at MAX_ARRAY_SIZE. Separately, java.lang.AbstractStringBuilder#MAX_ARRAY_SIZE seems unused; I wonder how that happened. -Pavel > On 9 Oct 2021, at 11:38, Andrey Turbanov wrote: > > Hello. > I came across unexpected behaviour of Arrays.deepToString method. > It throws OOM even on non-huge arrays. > For example this code: > >int size = Integer.MAX_VALUE / 19; >Integer[] integers = new Integer[size]; >Arrays.fill(integers, 0); > System.out.println(Arrays.deepToString(integers)); > > Fails with following stack trace: > > Exception in thread "main" java.lang.OutOfMemoryError: Requested array > size exceeds VM limit >at > java.base/java.lang.AbstractStringBuilder.(AbstractStringBuilder.java:86) >at java.base/java.lang.StringBuilder.(StringBuilder.java:112) >at java.base/java.util.Arrays.deepToString(Arrays.java:5160) > > > I believe it should be able to handle such arrays and not throw OOM > > > Andrey Turbanov
Re: RFR: 8274544: Langtools command's usage were grabled on Japanese Windows
On Thu, 30 Sep 2021 09:36:34 GMT, Ichiroh Takiguchi wrote: > JEP-400 (UTF-8 by Default) was eabled on JDK18-b13. > After JDK18-b13, javac and some other langtool command's usage were garbled > on Japanese Windows. > These commands use PrintWriter instead of standard out/err with PrintStream. For searchability, you could fix the typo in the PR title and JBS summary: grABled -> gARbled. A nit, really. - PR: https://git.openjdk.java.net/jdk/pull/5771
Integrated: 8274367: Re-indent stack-trace examples for Throwable.printStackTrace
On Mon, 27 Sep 2021 12:04:48 GMT, Pavel Rappo wrote: > This PR fixes indentation in the examples in the doc comment for > java.lang.Throwable#printStackTrace(). > > 1. Indentation in stack-trace output is significant. The cause of an > exception is output on the same level of indentation as that of the > exception. A suppressed exception is output at a deeper level of indentation > than that of the containing exception. The last example for > Throwable.printStackTrace violates this. > > 2. Indentation in examples for Throwable.printStackTrace that relate to > suppressed exceptions is inconsistent with that of cause exceptions. This pull request has now been integrated. Changeset: c880b87a Author:Pavel Rappo URL: https://git.openjdk.java.net/jdk/commit/c880b87a205cc9611fe88cb29f506293dfebf946 Stats: 21 lines in 1 file changed: 0 ins; 0 del; 21 mod 8274367: Re-indent stack-trace examples for Throwable.printStackTrace Reviewed-by: mchung, iris, darcy, bpb - PR: https://git.openjdk.java.net/jdk/pull/5715
RFR: 8274367: Re-indent stack-trace examples for Throwable.printStackTrace
This PR fixes indentation in the examples in the doc comment for java.lang.Throwable#printStackTrace(). 1. Indentation in stack-trace output is significant. The cause of an exception is output on the same level of indentation as that of the exception. A suppressed exception is output at a deeper level of indentation than that of the containing exception. The last example for Throwable.printStackTrace violates this. 2. Indentation in examples for Throwable.printStackTrace that relate to suppressed exceptions is inconsistent with that of cause exceptions. - Commit messages: - Initial commit Changes: https://git.openjdk.java.net/jdk/pull/5715/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5715=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274367 Stats: 21 lines in 1 file changed: 0 ins; 0 del; 21 mod Patch: https://git.openjdk.java.net/jdk/pull/5715.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5715/head:pull/5715 PR: https://git.openjdk.java.net/jdk/pull/5715
Integrated: 8274075: Fix miscellaneous typos in java.base
On Tue, 21 Sep 2021 12:26:25 GMT, Pavel Rappo wrote: > 8274075: Fix miscellaneous typos in java.base This pull request has now been integrated. Changeset: 87998565 Author: Pavel Rappo URL: https://git.openjdk.java.net/jdk/commit/8799856528f5804b616b734caed3fc4ba9811bfa Stats: 26 lines in 9 files changed: 0 ins; 1 del; 25 mod 8274075: Fix miscellaneous typos in java.base Reviewed-by: dfuchs, darcy, iris, lancea, bpb - PR: https://git.openjdk.java.net/jdk/pull/5610
Re: RFR: 8274075: Fix miscellaneous typos in java.base [v4]
> 8274075: Fix miscellaneous typos in java.base Pavel Rappo has updated the pull request incrementally with one additional commit since the last revision: Add missing "the" (Spotted by Brian Burkhalter.) - Changes: - all: https://git.openjdk.java.net/jdk/pull/5610/files - new: https://git.openjdk.java.net/jdk/pull/5610/files/05505d97..fa81cacd Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5610=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5610=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5610.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5610/head:pull/5610 PR: https://git.openjdk.java.net/jdk/pull/5610
Integrated: 8274071: Clean up java.lang.ref comments and documentation
On Tue, 21 Sep 2021 12:05:27 GMT, Pavel Rappo wrote: > This PR fixes an inline comment typo and reduces "overlinking" in a doc > comment in `java.lang.ref.Reference`. Overlinking happens because the > `reachabilityFence` method: > * Links `package-summary.html#reachability` twice. > * Refers to JLS 12.6 twice: once from a block tag and once from an inline > tag. This pull request has now been integrated. Changeset: c6df3c95 Author:Pavel Rappo URL: https://git.openjdk.java.net/jdk/commit/c6df3c9571cfa9607f3deffeaa77701dde9fea15 Stats: 4 lines in 1 file changed: 0 ins; 2 del; 2 mod 8274071: Clean up java.lang.ref comments and documentation Reviewed-by: rriggs, kbarrett, mchung, iris, lancea - PR: https://git.openjdk.java.net/jdk/pull/5609
Re: RFR: 8274075: Fix miscellaneous typos in java.base [v2]
On Tue, 21 Sep 2021 13:16:02 GMT, Pavel Rappo wrote: >> 8274075: Fix miscellaneous typos in java.base > > Pavel Rappo has updated the pull request incrementally with one additional > commit since the last revision: > > Tweak wording for Throwable constructor parameters Please re-review this change since I've added two commits. - PR: https://git.openjdk.java.net/jdk/pull/5610
Re: RFR: 8274075: Fix miscellaneous typos in java.base [v2]
On Wed, 22 Sep 2021 10:24:12 GMT, Pavel Rappo wrote: >> 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. > > I'm inclined to revert this change on Throwable.java:68, because of the lack > of consensus. It clearly is not a simple typo. Reverted in 57b71c5. - PR: https://git.openjdk.java.net/jdk/pull/5610
Re: RFR: 8274075: Fix miscellaneous typos in java.base [v3]
> 8274075: Fix miscellaneous typos in java.base Pavel Rappo has updated the pull request incrementally with two additional commits since the last revision: - Fix "non-white space" 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. - Undo "wrapped" -> "wrapping" - Changes: - all: https://git.openjdk.java.net/jdk/pull/5610/files - new: https://git.openjdk.java.net/jdk/pull/5610/files/19b6f496..05505d97 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5610=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5610=01-02 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/5610.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5610/head:pull/5610 PR: https://git.openjdk.java.net/jdk/pull/5610
Re: RFR: 8274075: Fix miscellaneous typos in java.base [v2]
On Tue, 21 Sep 2021 22:00:29 GMT, David Holmes wrote: >> 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"? > > 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. I'm inclined to revert this change on Throwable.java:68, because of the lack of consensus. It clearly is not a simple typo. - PR: https://git.openjdk.java.net/jdk/pull/5610
Re: RFR: 8274075: Fix miscellaneous typos in java.base [v2]
On Tue, 21 Sep 2021 16:56:03 GMT, Lance Andersen wrote: >> src/java.base/share/classes/java/lang/Throwable.java line 68: >> >>> 66: * Further, doing so would tie the API of the upper layer to the >>> details of >>> 67: * its implementation, assuming the lower layer's exception was a >>> checked >>> 68: * exception. Throwing a "wrapping exception" (i.e., an exception >>> containing a >> >> Are we sure that this should be "wrapping" and not "wrapped?" See also for >> example `java.security.cert.CertPathValidatorException.` > > It does seem a bit strange to say "Throwing a wrapping" 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". - PR: https://git.openjdk.java.net/jdk/pull/5610
Re: RFR: 8274075: Fix miscellaneous typos in java.base [v2]
On Tue, 21 Sep 2021 17:14:31 GMT, Pavel Rappo wrote: >> Subjectively, "wrapping exception" would seem to be an exception in the >> process of wrapping something. > > Would "wrappER" be better? 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: https://github.com/openjdk/jdk/blob/19b6f49649a317e6b255aeb810c8d533119b93bb/src/java.base/share/classes/java/lang/Throwable.java#L548-L554 - PR: https://git.openjdk.java.net/jdk/pull/5610
Re: RFR: 8274075: Fix miscellaneous typos in java.base [v2]
On Tue, 21 Sep 2021 17:10:02 GMT, Brian Burkhalter wrote: >> 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". > > Subjectively, "wrapping exception" would seem to be an exception in the > process of wrapping something. Would "wrappER" be better? - PR: https://git.openjdk.java.net/jdk/pull/5610
Re: RFR: 8274079: Cleanup unnecessary calls to Throwable.initCause() in java.base module
On Thu, 16 Sep 2021 19:03:26 GMT, Andrey Turbanov wrote: > Pass "cause" exception as constructor parameter is shorter and easier to read. This will need to be thoroughly tested to make sure there were no implicit dependencies on now-removed happens-before edges (`initCause` is synchronized). That said, the idea behind this clean-up looks good. - PR: https://git.openjdk.java.net/jdk/pull/5551
Re: RFR: 8274075: Fix miscellaneous typos in java.base [v2]
> 8274075: Fix miscellaneous typos in java.base Pavel Rappo has updated the pull request incrementally with one additional commit since the last revision: Tweak wording for Throwable constructor parameters - Changes: - all: https://git.openjdk.java.net/jdk/pull/5610/files - new: https://git.openjdk.java.net/jdk/pull/5610/files/2db8525d..19b6f496 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5610=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5610=00-01 Stats: 3 lines in 1 file changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5610.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5610/head:pull/5610 PR: https://git.openjdk.java.net/jdk/pull/5610
RFR: 8274075: Fix miscellaneous typos in java.base
8274075: Fix miscellaneous typos in java.base - Commit messages: - Initial commit Changes: https://git.openjdk.java.net/jdk/pull/5610/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5610=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274075 Stats: 21 lines in 8 files changed: 0 ins; 0 del; 21 mod Patch: https://git.openjdk.java.net/jdk/pull/5610.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5610/head:pull/5610 PR: https://git.openjdk.java.net/jdk/pull/5610
RFR: 8274071: Clean up java.lang.ref comments and documentation
This PR fixes an inline comment typo and reduces "overlinking" in a doc comment in `java.lang.ref.Reference`. Overlinking happens because the `reachabilityFence` method: * Links `package-summary.html#reachability` twice. * Refers to JLS 12.6 twice: once from a block tag and once from an inline tag. - Commit messages: - Initial commit Changes: https://git.openjdk.java.net/jdk/pull/5609/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5609=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274071 Stats: 4 lines in 1 file changed: 0 ins; 2 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5609.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5609/head:pull/5609 PR: https://git.openjdk.java.net/jdk/pull/5609
Integrated: 8273616: Fix trivial doc typos in the java.base module
On Fri, 10 Sep 2021 21:16:19 GMT, Pavel Rappo wrote: > 8273616: Fix trivial doc typos in the java.base module This pull request has now been integrated. Changeset: b4b12101 Author: Pavel Rappo URL: https://git.openjdk.java.net/jdk/commit/b4b121018d16e531f3a51ff75ae37fdc374d530b Stats: 55 lines in 34 files changed: 0 ins; 0 del; 55 mod 8273616: Fix trivial doc typos in the java.base module Reviewed-by: jrose, iris, lancea, dfuchs, rriggs - PR: https://git.openjdk.java.net/jdk/pull/5475
Re: RFR: 8273616: Fix trivial doc typos in the java.base module [v2]
On Fri, 10 Sep 2021 23:20:11 GMT, Pavel Rappo wrote: >> 8273616: Fix trivial doc typos in the java.base module > > Pavel Rappo has updated the pull request incrementally with one additional > commit since the last revision: > > Revert two fixes Thanks to those who reviewed this PR. Since I posted it, I've found three more occurrences of _insure_ used in the sense of _ensure_: two in the `java.io.Object*Stream` area and one in the `java.util.Currency` class. I decided to fix those in this PR, which now needs to be (re)reviewed. Thanks! There are more occurrences of _insure_, which I didn't touch. Some of them are in java.sql, java.sql.rowset and java.desktop. In the latter, _insure_ even crept into method names. - PR: https://git.openjdk.java.net/jdk/pull/5475
Re: RFR: 8273616: Fix trivial doc typos in the java.base module [v3]
> 8273616: Fix trivial doc typos in the java.base module Pavel Rappo has updated the pull request incrementally with one additional commit since the last revision: Use "ensure" instead of "insure" - Changes: - all: https://git.openjdk.java.net/jdk/pull/5475/files - new: https://git.openjdk.java.net/jdk/pull/5475/files/9a9deee1..4b33fb94 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5475=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5475=01-02 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/5475.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5475/head:pull/5475 PR: https://git.openjdk.java.net/jdk/pull/5475
Re: RFR: 8273616: Fix trivial doc typos in the java.base module [v2]
> 8273616: Fix trivial doc typos in the java.base module Pavel Rappo has updated the pull request incrementally with one additional commit since the last revision: Revert two fixes - Changes: - all: https://git.openjdk.java.net/jdk/pull/5475/files - new: https://git.openjdk.java.net/jdk/pull/5475/files/b90d1556..9a9deee1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5475=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5475=00-01 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/5475.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5475/head:pull/5475 PR: https://git.openjdk.java.net/jdk/pull/5475
Re: RFR: 8273616: Fix trivial doc typos in the java.base module [v2]
On Fri, 10 Sep 2021 21:52:36 GMT, John R Rose wrote:
>> Pavel Rappo has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Revert two fixes
>
> src/java.base/share/classes/java/nio/channels/FileChannel.java line 567:
>
>> 565: * If {@code true} then this method is required to force
>> changes
>> 566: * to both the file's content and metadata to be written to
>> 567: * storage; otherwise, it needs only force content changes
>> to be
>
> (same as previous comment: the suggested fix makes the English *less*
> correct)
Reverted both in 9a9deee; thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/5475
RFR: 8273616: Fix trivial doc typos in the java.base module
8273616: Fix trivial doc typos in the java.base module - Commit messages: - Initial commit Changes: https://git.openjdk.java.net/jdk/pull/5475/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5475=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273616 Stats: 55 lines in 34 files changed: 0 ins; 0 del; 55 mod Patch: https://git.openjdk.java.net/jdk/pull/5475.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5475/head:pull/5475 PR: https://git.openjdk.java.net/jdk/pull/5475
Re: Possible ClassCastException in java.util.regex.Pattern.BmpCharPredicate#union(java.util.regex.Pattern.CharPredicate...)
Has that method been ever used? If nothing else its name seems strange. To me,
a union has OR semantics, not AND.
> On 27 Aug 2021, at 15:37, Andrey Turbanov wrote:
>
> Hello.
> I found suspicious code in the method
> "java.util.regex.Pattern.BmpCharPredicate#union(java.util.regex.Pattern.CharPredicate...)"
> https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/regex/Pattern.java#L5639
>
> static CharPredicate union(CharPredicate... predicates) {
>CharPredicate cp = ch -> {
>for (CharPredicate p : predicates) {
>if (!p.is(ch))
>return false;
>}
>return true;
>};
>for (CharPredicate p : predicates) {
>if (! (p instanceof BmpCharPredicate))
>return cp;
>}
>return (BmpCharPredicate)cp;
> }
>
> Variable `cp` has type CharPredicate initially. And then it's casted
> to BmpCharPredicate. This cast always fails with ClassCastException
> when reached.
>
> Can reproduced in small sample class:
>
>public static void main(String[] args) {
>CharPredicate result = BmpCharPredicate.union();
>System.out.println(result);
>}
>
>interface CharPredicate {
>boolean is(int ch);
>}
>
>interface BmpCharPredicate extends CharPredicate {
>static CharPredicate union(CharPredicate... predicates) {
>CharPredicate cp = ch -> true;
>for (CharPredicate p : predicates) {
>if (! (p instanceof BmpCharPredicate))
>return cp;
>}
>return (BmpCharPredicate)cp;
>}
>}
>
>
> Exception in thread "main" java.lang.ClassCastException: class
> org.RegexpBug$BmpCharPredicate$$Lambda$14/0x000800c028f0 cannot be
> cast to class org.RegexpBug$BmpCharPredicate
> (org.RegexpBug$BmpCharPredicate$$Lambda$14/0x000800c028f0 and
> org.RegexpBug$BmpCharPredicate are in unnamed module of loader 'app')
>at org.RegexpBug$BmpCharPredicate.union(RegexpBug.java:20)
>at org.RegexpBug.main(RegexpBug.java:5)
>
> As I can see this method is never used. Perhaps it should be removed?
>
>
> Andrey Turbanov
Re: Dangling class-level javadoc comments in JDK
Just to clarify that example of mine. I didn't mean the package-info.java file. I meant an ordinary file that comprises an interface, class, enum or record. Perhaps a better example from Andrey's list would be this: /** * A utility package for the java(1), javaw(1) launchers. * The following are helper methods that the native launcher uses * to perform checks etc. using JNI, see src/share/bin/java.c */ import java.io.File; In no context can an import statement be documented. -Pavel > On 24 Aug 2021, at 16:07, Pavel Rappo wrote: > >> On 24 Aug 2021, at 15:38, Jonathan Gibbons >> wrote: >> >> IIRC, the one in javadoc CommentUtils has recently been fixed. >> >> It might be worth a javac -Xlint option to detect/report such dangling >> comments. > > How would you currently implement that? Aren't comments on non-documentable > constructs discarded early? At what point during compilation and how would > you detect, for example, that this doc comment dangles? > >/** > * IOUtils: A collection of IO-related public static methods. > */ > >package sun.security.util; > > -Pavel > >> -- Jon >> >> On 8/23/21 11:50 PM, Andrey Turbanov wrote: >>> Hello. >>> I found a few internal classes in the JDK codebase which don't have >>> proper javadoc, but have dangling javadoc-like comments. >>> Dangling Javadoc comments are ignored by the javadoc tool and IDE. >>> Perhaps it was intentional to not add proper javadoc to them? >>> I believe it's better to convert them to proper javadoc to make >>> developing JDK a bit more friendly within IDE. >>> What do you think? >>> >>> List of classes: >>> >>> com.sun.beans.editors.BooleanEditor >>> com.sun.beans.editors.ByteEditor >>> com.sun.beans.editors.DoubleEditor >>> com.sun.beans.editors.FloatEditor >>> com.sun.beans.editors.IntegerEditor >>> com.sun.beans.editors.LongEditor >>> com.sun.beans.editors.NumberEditor >>> com.sun.beans.editors.ShortEditor >>> com.sun.jndi.toolkit.dir.ContainmentFilter >>> com.sun.jndi.toolkit.dir.LazySearchEnumerationImpl >>> com.sun.security.auth.module.Crypt >>> java.math.MutableBigInteger >>> java.net.DefaultInterface >>> javax.swing.GraphicsWrapper >>> jdk.internal.access.JavaAWTFontAccess >>> jdk.javadoc.internal.doclets.toolkit.CommentUtils >>> sun.awt.X11.XAtom >>> sun.awt.X11.XAwtState >>> sun.java2d.xr.XRBackend >>> sun.java2d.xr.XRDrawLine >>> sun.jvm.hotspot.debugger.PageCache >>> sun.jvm.hotspot.runtime.JavaThreadFactory >>> sun.jvm.hotspot.utilities.Interval >>> sun.jvm.hotspot.utilities.MessageQueueBackend >>> sun.jvm.hotspot.utilities.RBTree >>> sun.launcher.LauncherHelper >>> sun.net.www.content.text.plain >>> sun.net.www.protocol.file.FileURLConnection >>> sun.nio.ch.Interruptible >>> sun.security.pkcs.ParsingException >>> sun.security.provider.SeedGenerator >>> sun.security.util.ByteArrayTagOrder >>> sun.security.util.IOUtils >>> >>> >>> Andrey Turbanov >
Re: Dangling class-level javadoc comments in JDK
> On 24 Aug 2021, at 15:38, Jonathan Gibbons > wrote: > > IIRC, the one in javadoc CommentUtils has recently been fixed. > > It might be worth a javac -Xlint option to detect/report such dangling > comments. How would you currently implement that? Aren't comments on non-documentable constructs discarded early? At what point during compilation and how would you detect, for example, that this doc comment dangles? /** * IOUtils: A collection of IO-related public static methods. */ package sun.security.util; -Pavel > -- Jon > > On 8/23/21 11:50 PM, Andrey Turbanov wrote: >> Hello. >> I found a few internal classes in the JDK codebase which don't have >> proper javadoc, but have dangling javadoc-like comments. >> Dangling Javadoc comments are ignored by the javadoc tool and IDE. >> Perhaps it was intentional to not add proper javadoc to them? >> I believe it's better to convert them to proper javadoc to make >> developing JDK a bit more friendly within IDE. >> What do you think? >> >> List of classes: >> >> com.sun.beans.editors.BooleanEditor >> com.sun.beans.editors.ByteEditor >> com.sun.beans.editors.DoubleEditor >> com.sun.beans.editors.FloatEditor >> com.sun.beans.editors.IntegerEditor >> com.sun.beans.editors.LongEditor >> com.sun.beans.editors.NumberEditor >> com.sun.beans.editors.ShortEditor >> com.sun.jndi.toolkit.dir.ContainmentFilter >> com.sun.jndi.toolkit.dir.LazySearchEnumerationImpl >> com.sun.security.auth.module.Crypt >> java.math.MutableBigInteger >> java.net.DefaultInterface >> javax.swing.GraphicsWrapper >> jdk.internal.access.JavaAWTFontAccess >> jdk.javadoc.internal.doclets.toolkit.CommentUtils >> sun.awt.X11.XAtom >> sun.awt.X11.XAwtState >> sun.java2d.xr.XRBackend >> sun.java2d.xr.XRDrawLine >> sun.jvm.hotspot.debugger.PageCache >> sun.jvm.hotspot.runtime.JavaThreadFactory >> sun.jvm.hotspot.utilities.Interval >> sun.jvm.hotspot.utilities.MessageQueueBackend >> sun.jvm.hotspot.utilities.RBTree >> sun.launcher.LauncherHelper >> sun.net.www.content.text.plain >> sun.net.www.protocol.file.FileURLConnection >> sun.nio.ch.Interruptible >> sun.security.pkcs.ParsingException >> sun.security.provider.SeedGenerator >> sun.security.util.ByteArrayTagOrder >> sun.security.util.IOUtils >> >> >> Andrey Turbanov
Re: RFR: 8271302: Regex Test Refresh [v3]
On Fri, 20 Aug 2021 16:49:15 GMT, Pavel Rappo wrote: >> Ian Graves has updated the pull request incrementally with one additional >> commit since the last revision: >> >> some quick fixes > > test/jdk/java/util/regex/RegExTest.java line 4270: > >> 4268: String s = (String)pm[1]; >> 4269: boolean r = (Boolean)pm[2]; >> 4270: assertSame(r, Pattern.compile(p).matcher(s).matches()); > > `expectThrows(PatternSyntaxException.class, ...)` might suit better for this > and similar cases in this file. Not only does `expectThrows` test for > expected exception, it also returns an exception which you can further > inspect. Now, if we only had `assertThat` or similar functionality for > compound assertions, the complete test might've looked nicely. The latter comment referred to hand-rolled try-fail constructs like this: https://github.com/openjdk/jdk/blob/3937bf8f9921395d6fcbdc2bb00a4a98dc2aaf62/test/jdk/java/util/regex/RegExTest.java#L4281-L4289 Not sure why my IDE plugin messed up the comment position. - PR: https://git.openjdk.java.net/jdk/pull/5092
Re: RFR: 8271302: Regex Test Refresh [v3]
On Fri, 20 Aug 2021 16:27:53 GMT, Ian Graves wrote: >> 8271302: Regex Test Refresh > > Ian Graves has updated the pull request incrementally with one additional > commit since the last revision: > > some quick fixes test/jdk/java/util/regex/RegExTest.java line 4270: > 4268: String s = (String)pm[1]; > 4269: boolean r = (Boolean)pm[2]; > 4270: assertSame(r, Pattern.compile(p).matcher(s).matches()); `assertEquals(boolean, boolean)` might be better. test/jdk/java/util/regex/RegExTest.java line 4270: > 4268: String s = (String)pm[1]; > 4269: boolean r = (Boolean)pm[2]; > 4270: assertSame(r, Pattern.compile(p).matcher(s).matches()); `expectThrows(PatternSyntaxException.class, ...)` might suit better for this and similar cases in this file. Not only does `expectThrows` test for expected exception, it also returns an exception which you can further inspect. Now, if we only had `assertThat` or similar functionality for compound assertions, the complete test might've looked nicely. - PR: https://git.openjdk.java.net/jdk/pull/5092
Re: RFR: 8271302: Regex Test Refresh [v2]
On Fri, 20 Aug 2021 13:46:39 GMT, Pavel Rappo wrote:
>> Ian Graves has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Couple of fixes
>
> test/jdk/java/util/regex/NegativeArraySize.java line 40:
>
>> 38: @Test
>> 39: public static void testNegativeArraySize() {
>> 40: assertThrows(OutOfMemoryError.class, () -> Pattern.compile("\\Q"
>> + "a".repeat(42 + Integer.MAX_VALUE / 3)));
>
> One observation on this regex. Although the regex looks invalid because `\\Q`
> misses the pairing `\\E`, it can still be compiled (with a reasonable number
> of a's, of course). Moreover, the resulting pattern matches strings in a
> surprising way:
>
>
> jshell> Pattern.compile("\\Qaaa").matcher("aaa").matches()
> $1 ==> true
Maybe that behavior is expected after all. From "Mastering Regular Expressions"
by Jeffrey E.F. Friedl, 3rd Edition, p. 136:
> Literal-text span: `\Q...\E`
>
> First introduced with Perl, the special sequence `\Q...\E` turns off all
> regex meta-characters between them, except for `\E` itself. (If the `\E` is
> omitted, they are turned off until the end of the regex.)
-
PR: https://git.openjdk.java.net/jdk/pull/5092
Re: RFR: 8271302: Regex Test Refresh [v2]
On Wed, 18 Aug 2021 18:35:53 GMT, Ian Graves wrote:
>> 8271302: Regex Test Refresh
>
> Ian Graves has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Couple of fixes
test/jdk/java/util/regex/NegativeArraySize.java line 2:
> 1: /*
> 2: * Copyright (c) 2019, 2021 Oracle and/or its affiliates. All rights
> reserved.
Add comma after 2021, or the copyright headers check won't pass.
test/jdk/java/util/regex/NegativeArraySize.java line 29:
> 27: * @summary Pattern.compile() can throw confusing
> NegativeArraySizeException
> 28: * @requires os.maxMemory >= 5g
> 29: * @run testng/othervm -Xms5G -Xmx5G NegativeArraySize
I note that the order of the arguments has changed. Will that work as expected?
Had it worked as expected before?
test/jdk/java/util/regex/NegativeArraySize.java line 40:
> 38: @Test
> 39: public static void testNegativeArraySize() {
> 40: assertThrows(OutOfMemoryError.class, () -> Pattern.compile("\\Q"
> + "a".repeat(42 + Integer.MAX_VALUE / 3)));
One observation on this regex. Although the regex looks invalid because `\\Q`
misses the pairing `\\E`, it can still be compiled (with a reasonable number of
a's, of course). Moreover, the resulting pattern matches strings in a
surprising way:
jshell> Pattern.compile("\\Qaaa").matcher("aaa").matches()
$1 ==> true
test/jdk/java/util/regex/RegExTest.java line 27:
> 25: * @test
> 26: * @summary tests RegExp framework (use -Dseed=X to set PRNG seed)
> 27: * @author Mike McCloskey
What happened to Mike here? :-)
test/jdk/java/util/regex/RegExTest.java line 121:
> 119: private static void check(String p, String s, boolean expected) {
> 120: Matcher matcher = Pattern.compile(p).matcher(s);
> 121: assertSame(matcher.find(), expected);
Why use `assertSame(Object, Object)`? I would expect `assertEquals(boolean,
boolean)`.
test/jdk/java/util/regex/RegExTest.java line 206:
> 204: }
> 205:
> 206: // Regular expression test// Most of the tests are in a file
Mistakenly joined comment lines?
-
PR: https://git.openjdk.java.net/jdk/pull/5092
Re: RFR: 8269753: Misplaced caret in PatternSyntaxException's detail message [v3]
On Mon, 26 Jul 2021 20:54:53 GMT, Ian Graves wrote: >> Fixes a bug where carets aren't indented correctly in PatternSyntaxException >> messages because tab characters are converted to spaces in their indentation. > > Ian Graves has updated the pull request incrementally with one additional > commit since the last revision: > > Tweaking some spacing. Fixing some report placement. Marked as reviewed by prappo (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4906
Re: RFR: 8269753: Misplaced caret in PatternSyntaxException's detail message
On Mon, 26 Jul 2021 18:10:03 GMT, Ian Graves wrote:
> Fixes a bug where carets aren't indented correctly in PatternSyntaxException
> messages because tab characters are converted to spaces in their indentation.
Changes requested by prappo (Reviewer).
src/java.base/share/classes/java/util/regex/PatternSyntaxException.java line
111:
> 109: sb.append(System.lineSeparator());
> 110: for (int i = 0; i < index; i++) {
> 111: sb.append((pattern.charAt(i) == '\t') ? '\t' : ' ');
In contrast with
https://github.com/igraves/jdk/blob/2609dd9618dd43ea0de9abe3e3100262d09c079c/src/jdk.compiler/share/classes/com/sun/tools/javac/util/AbstractDiagnosticFormatter.java#L324,
this code uses `StringBuilder.append(char)`, which might be even cleaner; good.
test/jdk/java/util/regex/RegExTest.java line 5304:
> 5302: var message = e.getMessage();
> 5303: var sep = System.lineSeparator();
> 5304: if(message.contains(sep + "\t ^")){
Suggestion:
if (message.contains(sep + "\t ^")) {
test/jdk/java/util/regex/RegExTest.java line 5310:
> 5308: failCount++;
> 5309:
> 5310: report("Correct caret indentation for patterns with tabs");
`report` will not be reached on success; this is different from how `report` is
used in the rest of the file. Also: `report` seems to be stateful in that it
resets the `failCount`.
-
PR: https://git.openjdk.java.net/jdk/pull/4906
Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message [v5]
On Tue, 2 Mar 2021 18:07:24 GMT, Stuart Marks wrote:
>> test/jdk/jdk/internal/util/ArraysSupport/NewLength.java line 100:
>>
>>> 98: int r = ArraysSupport.newLength(old, min, pref);
>>> 99: fail("expected OutOfMemoryError, got normal return value of
>>> " + r);
>>> 100: } catch (OutOfMemoryError success) { }
>>
>> Consider using `expectThrows` or `assertThrows` from `org.testng.Assert`.
>
> Good point. However, I actually tried to use assertThrows, but there doesn't
> seem to be a way to get the unexpected return value into the error message. I
> think having this value in the test output is important for diagnosing
> failures.
That tradeoff you made, makes sense.
Indeed, neither `assertThrows` nor `expectThrows` in `org.testng.Assert` logs
an unexpectedly returned value. This is because the code being tested is
expressed as the `Assert.ThrowingRunnable` interface which has a single `void`
method. So the code being tested cannot return a value to the testing framework.
Now, if `org.testng.Assert` were to provide the respective methods accepting,
say, `Assert.ThrowingCallable` that had a value-returning method, TestNG would
run into the same issues that JUnit ran into when they attempted to do that:
https://github.com/junit-team/junit5/issues/1576
-
PR: https://git.openjdk.java.net/jdk/pull/1617
Re: RFR: 8199594: Add doc describing how (?x) ignores spaces in character classes
On Mon, 28 Jun 2021 20:50:42 GMT, Ian Graves wrote:
> 8199594: Add doc describing how (?x) ignores spaces in character classes
src/java.base/share/classes/java/util/regex/Pattern.java line 833:
> 831: * Comments mode can also be enabled via the embedded flag
> 832: * expression{@code (?x)}.
> 833: *
Nit: is this extra line necessary?
-
PR: https://git.openjdk.java.net/jdk/pull/4618
Re: [jdk17] RFR: 8269704: Typo in j.t.Normalizer.normalize()
On Wed, 30 Jun 2021 21:38:43 GMT, Naoto Sato wrote: > A trivial typo fix. Marked as reviewed by prappo (Reviewer). - PR: https://git.openjdk.java.net/jdk17/pull/187
Re: [jdk17] RFR: 8268080: java/util/concurrent/forkjoin/AsyncShutdownNow.java fails with java.util.concurrent.RejectedExecutionException
On Wed, 16 Jun 2021 09:57:29 GMT, Julia Boes wrote: > In the methods in question, `RejectedExecutionException` is an expected > exception that was previously unhandled (it is a `RuntimeException`, not a > subclass of `ExecutionException`). This change adds > `RejectedExecutionException` to the existing catch clause. test/jdk/java/util/concurrent/forkjoin/AsyncShutdownNowInvokeAny.java line 2: > 1: /* > 2: * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved. Why did you replace the year rather than append it? - PR: https://git.openjdk.java.net/jdk17/pull/74
Re: [jdk17] RFR: 8268736: Use apiNote in AutoCloseable.close javadoc
On Tue, 15 Jun 2021 18:05:18 GMT, Joe Darcy wrote: > The javadoc of AutoCloseable.close is from JDK 7 and thus predates tags like > @apiNote. However, some of the discussion contained in AutoCloseable.close is > more appropriate as an apiNote that normal text. I'm confused: I thought that both https://openjdk.java.net/jeps/8068562 and the actual usages of `@apiNote` in `java.base` accumulated since then are advisory to the API users rather than to API implementors. - PR: https://git.openjdk.java.net/jdk17/pull/63
Re: java.util.List missing from "Collections Framework Overview" javadoc
I have created a JBS issue: https://bugs.openjdk.java.net/browse/JDK-8268077 > On 1 Jun 2021, at 18:56, Raffaello Giulietti > wrote: > > Hi, > > can anybody take a look at this? > I would do it myself but I don't have Author status to add an issue to the > JBS. > > Should be a trivial change. > > Thanks > Raffaello > > > On 2021-05-30 18:03, Raffaello Giulietti wrote: >> Hello, >> seems like java.util.List is missing from the list in the "Collection >> Interfaces" section in >> https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/util/doc-files/coll-overview.html >> Should be easy to fix and doesn't seem to require a CSR as the document is >> non-normative. >> Greetings >> Raffaello
Re: RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
On Mon, 10 May 2021 06:34:36 GMT, Vyom Tewari wrote: > this change will include the below headers files to Linux only. > #include > #include The fix to 8266610 caused a build failure on macOS. What is your plan to make sure this won't happen with this change? I would recommend enabling [GitHub Actions](https://docs.github.com/en/github/administering-a-repository/disabling-or-limiting-github-actions-for-a-repository) for the "vyommani/jdk" repo. I have run this PR through our CI: the tier1 tests are fine on all supported platforms. Please integrate this PR as soon as possible. Separately, please sort out the way you test your PRs. Read the error message that the failed GitHub Actions gave you: > Prerequisites > Actions jobs for this repository have been disabled by GitHub staff. If you > are the repository owner, you can contact support via github.com/contact for > more information. - PR: https://git.openjdk.java.net/jdk/pull/3943
