Re: [jdk19] RFR: 8289779: Map::replaceAll javadoc has redundant @throws clauses
On Wed, 6 Jul 2022 00:32:00 GMT, Stuart Marks wrote: > Simple javadoc fix of an editorial nature. Thanks for doing this, Stuart. Not only does this PR remove duplication from the `Map` documentation, but it also ensures that when JDK-6509045 is integrated, that duplication won't spread to `ConcurrentMap`. A bonus of this fix is that the irrelevant mention of "keys" is gone from the description of `NullPointerException`. Also, consider closing 8255426 as a duplicate. - Marked as reviewed by prappo (Reviewer). PR: https://git.openjdk.org/jdk19/pull/111
Re: [jdk19] RFR: 8289779: Map::replaceAll javadoc has redundant @throws clauses [v2]
On Wed, 6 Jul 2022 23:03:42 GMT, Stuart Marks wrote: >> Simple javadoc fix of an editorial nature. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Reflow and adjust whitespace per Alan's & Iris' comments. Marked as reviewed by prappo (Reviewer). - PR: https://git.openjdk.org/jdk19/pull/111
RFR: 8294377: Prepare to stop auto-inheriting documentation for subclasses of exceptions whose documentation is inherited
This adds exception documentation to JDK methods that would otherwise lose that
documentation once JDK-8287796 is integrated. While adding this exception
documentation now does not change [^1] the JDK API Documentation, it will
automatically counterbalance the effect that JDK-8287796 will have.
JDK-8287796 seeks to remove an old, undocumented, and esoteric javadoc feature
that cannot be suppressed [^2]. The feature is so little known about that IDEs
either do not implement it correctly or do not implement it at all, thus
rendering documentation differently from how the javadoc tool renders it.
That feature was introduced in JDK-4947455 and manifests as follows. If a
method inherits documentation for an exception, along with that documentation
the method automatically inherits documentation for all exceptions that are
subclasses of that former exception and are documented in an overridden method.
To illustrate that behavior, consider the following example. A method
`Subtype.m` inherits documentation for `SuperException`, while the overridden
method `Supertype.m` documents `SuperException`, `SubExceptionA` and
`SubExceptionB`, where the latter two exceptions are subclasses of the former
exception:
public class Subtype extends Supertype {
@Override
public void m() throws SuperException {
...
public class Supertype {
/**
* @throws SuperException general problem
* @throws SubExceptionA specific problem A
* @throws SubExceptionB specific problem B
*/
public void m() throws SuperException {
...
public class SuperException extends Exception {
...
public class SubExceptionA extends SuperException {
...
public class SubExceptionB extends SuperException {
...
For the above example, the API Documentation for `Subtype.m` will contain
documentation for all three exceptions; the page fragment for `Subtype.m` in
"Method Details" will look like this:
public void m()
throws SuperException
Overrides:
m in class Supertype
Throws:
SuperException - general problem
SubExceptionA - specific problem A
SubExceptionB - specific problem B
It works for checked and unchecked exceptions, for methods in classes and
interfaces.
If it's the first time you hear about that behavior and this change affects
your area, it's a good opportunity to reflect on whether the exception
documentation this change adds is really needed or you were simply unaware of
the fact that it was automatically added before. If exception documentation is
not needed, please comment on this PR. Otherwise, consider approving it.
Keep in mind that if some exception documentation is not needed, **leaving it
out** from this PR might require a CSR.
[^1]: Aside from insignificant changes on class-use and index pages. There's
one relatively significant change. This change is in jdk.jshell, where some
methods disappeared from "Methods declared in ..." section and other
irregularities. We are aware of this and have filed JDK-8291803 to fix the root
cause.
[^2]: In reality, the feature can be suppressed, but it looks like a bug rather
than intentional design. If we legitimize the feature and its suppression
behavior, the model for exception documentation inheritance might become much
more complicated.
-
Commit messages:
- Initial commit
Changes: https://git.openjdk.org/jdk/pull/10449/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=10449&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8294377
Stats: 322 lines in 15 files changed: 311 ins; 0 del; 11 mod
Patch: https://git.openjdk.org/jdk/pull/10449.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/10449/head:pull/10449
PR: https://git.openjdk.org/jdk/pull/10449
Re: RFR: 8294377: Prepare to stop auto-inheriting documentation for subclasses of exceptions whose documentation is inherited [v2]
> This adds exception documentation to JDK methods that would otherwise lose
> that documentation once JDK-8287796 is integrated. While adding this
> exception documentation now does not change [^1] the JDK API Documentation,
> it will automatically counterbalance the effect that JDK-8287796 will have.
>
> JDK-8287796 seeks to remove an old, undocumented, and esoteric javadoc
> feature that cannot be suppressed [^2]. The feature is so little known about
> that IDEs either do not implement it correctly or do not implement it at all,
> thus rendering documentation differently from how the javadoc tool renders it.
>
> That feature was introduced in JDK-4947455 and manifests as follows. If a
> method inherits documentation for an exception, along with that documentation
> the method automatically inherits documentation for all exceptions that are
> subclasses of that former exception and are documented in an overridden
> method.
>
> To illustrate that behavior, consider the following example. A method
> `Subtype.m` inherits documentation for `SuperException`, while the overridden
> method `Supertype.m` documents `SuperException`, `SubExceptionA` and
> `SubExceptionB`, where the latter two exceptions are subclasses of the former
> exception:
>
> public class Subtype extends Supertype {
>
> @Override
> public void m() throws SuperException {
> ...
>
> public class Supertype {
>
> /**
> * @throws SuperException general problem
> * @throws SubExceptionA specific problem A
> * @throws SubExceptionB specific problem B
> */
> public void m() throws SuperException {
> ...
>
> public class SuperException extends Exception {
> ...
>
> public class SubExceptionA extends SuperException {
> ...
>
> public class SubExceptionB extends SuperException {
> ...
>
> For the above example, the API Documentation for `Subtype.m` will contain
> documentation for all three exceptions; the page fragment for `Subtype.m` in
> "Method Details" will look like this:
>
> public void m()
>throws SuperException
>
> Overrides:
> m in class Supertype
> Throws:
> SuperException - general problem
> SubExceptionA - specific problem A
> SubExceptionB - specific problem B
>
> It works for checked and unchecked exceptions, for methods in classes and
> interfaces.
>
>
> If it's the first time you hear about that behavior and this change affects
> your area, it's a good opportunity to reflect on whether the exception
> documentation this change adds is really needed or you were simply unaware of
> the fact that it was automatically added before. If exception documentation
> is not needed, please comment on this PR. Otherwise, consider approving it.
>
> Keep in mind that if some exception documentation is not needed, **leaving it
> out** from this PR might require a CSR.
>
>
> [^1]: Aside from insignificant changes on class-use and index pages. There's
> one relatively significant change. This change is in jdk.jshell, where some
> methods disappeared from "Methods declared in ..." section and other
> irregularities. We are aware of this and have filed JDK-8291803 to fix the
> root cause.
> [^2]: In reality, the feature can be suppressed, but it looks like a bug
> rather than intentional design. If we legitimize the feature and its
> suppression behavior, the model for exception documentation inheritance might
> become much more complicated.
Pavel Rappo has updated the pull request incrementally with one additional
commit since the last revision:
revert an extraneous change to jdk.javadoc
-
Changes:
- all: https://git.openjdk.org/jdk/pull/10449/files
- new: https://git.openjdk.org/jdk/pull/10449/files/87bdbff4..d303f5ec
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=10449&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=10449&range=00-01
Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/10449.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/10449/head:pull/10449
PR: https://git.openjdk.org/jdk/pull/10449
Re: RFR: 8294377: Prepare to stop auto-inheriting documentation for subclasses of exceptions whose documentation is inherited [v2]
On Wed, 28 Sep 2022 13:34:08 GMT, Daniel Fuchs wrote:
>> Pavel Rappo has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> revert an extraneous change to jdk.javadoc
>
> src/java.naming/share/classes/javax/naming/directory/InitialDirContext.java
> line 186:
>
>> 184: /**
>> 185: * @throws AttributeModificationException {@inheritDoc}
>> 186: */
>
> Isn't `{@inheritDoc}` needed in this case to preserve the method & params
> description?
Elements of a method declaration such as formal and type parameters as well as
exceptions in the `throws` clause implicitly inherit descriptions, unless those
elements are described in that method declaration. The same goes for the main
description of a method: unless it exists in the method declaration, it is
inherited.
To sum up, those descriptions you ask about are implicitly inherited. No
additional javadoc markup is needed.
-
PR: https://git.openjdk.org/jdk/pull/10449
Re: RFR: 8294377: Prepare to stop auto-inheriting documentation for subclasses of exceptions whose documentation is inherited [v2]
On Wed, 28 Sep 2022 13:36:18 GMT, Daniel Fuchs wrote: > If you can assert that the generated docs are the same before and after the > proposed change then that's enough for me. Would it suffice if I show the result of the diff between JDK API Documentation built at the tip of this PR branch (docs) and that of the jdk/master commit (docs-old) from which this PR branches? If so, please have a look: % diff -qnr build/macosx-aarch64/images/docs build/macosx-aarch64/images/docs-old Files build/macosx-aarch64/images/docs/api/index-files/index-9.html and build/macosx-aarch64/images/docs-old/api/index-files/index-9.html differ Files build/macosx-aarch64/images/docs/api/java.base/java/lang/class-use/Exception.html and build/macosx-aarch64/images/docs-old/api/java.base/java/lang/class-use/Exception.html differ Files build/macosx-aarch64/images/docs/api/java.base/java/lang/class-use/Override.html and build/macosx-aarch64/images/docs-old/api/java.base/java/lang/class-use/Override.html differ Files build/macosx-aarch64/images/docs/api/java.base/java/lang/class-use/String.html and build/macosx-aarch64/images/docs-old/api/java.base/java/lang/class-use/String.html differ Files build/macosx-aarch64/images/docs/api/java.base/java/lang/reflect/class-use/Method.html and build/macosx-aarch64/images/docs-old/api/java.base/java/lang/reflect/class-use/Method.html differ Files build/macosx-aarch64/images/docs/api/jdk.jshell/jdk/jshell/execution/JdiDefaultExecutionControl.html and build/macosx-aarch64/images/docs-old/api/jdk.jshell/jdk/jshell/execution/JdiDefaultExecutionControl.html differ Files build/macosx-aarch64/images/docs/api/jdk.jshell/jdk/jshell/execution/RemoteExecutionControl.html and build/macosx-aarch64/images/docs-old/api/jdk.jshell/jdk/jshell/execution/RemoteExecutionControl.html differ Files build/macosx-aarch64/images/docs/api/member-search-index.js and build/macosx-aarch64/images/docs-old/api/member-search-index.js differ As I said in the PR description, the differences are insignificant and confined to class-use and index structures. Well, except for jdk.jshell. But even there it might be okay for now. - PR: https://git.openjdk.org/jdk/pull/10449
Re: RFR: 8294377: Prepare to stop auto-inheriting documentation for subclasses of exceptions whose documentation is inherited [v2]
On Fri, 30 Sep 2022 19:15:17 GMT, Phil Race wrote:
> If the docs end up the same as you say that is fine by me since for the two
> ImageIO classes that seems to be what we'd want.
Since this change does not affect HTML pages for java.desktop, I take it as
"approved".
> But inheritDoc behaviour is still "surprising". I've never been sure I
> understood it and now I'm just sure I don't.
>
> 1. The two ImageIO methods don't have @OverRide or _anything_ and yet javadoc
> infers
>they'd want to inherit the docs from the interface .. clever javadoc .. I
> guess I can see
>that some doc is better than none so this is OK
> 2. When it does inherit I'd expected that it copies the EXACT and ENTIRE
> javadoc
>from the super-interface, which for your change to work can't be all its
> doing.
>You've added JUST
>/**
>
> * @throws SomeExceptionSubType blah-blah
> */
>
> and yet javadoc somehow figures out this is to be ADDED to the super-type doc
> for the method and not replace it .. ???
>
> 3. What the old code was doing seems to me to be "natural" to the extent any
> of
>this does and the fix you are preparing would add to the surprising
> behaviours ..
Let me try to clarify exception documentation inheritance for you.
A method **never** inherits exception documentation unless that method
explicitly requests to do so. A method that wants to inherit documentation for
a particular exception has two options: use a doc comment or use a method
declaration.
Let's see how those options work. Suppose, a method wants to inherit
documentation for an exception `X`. To inherit documentation through a doc
comment, the method adds the following exception tag (`@throws` or
`@exception`) to that method's doc comment:
@throws X {@inheritDoc}
To inherit documentation through a method declaration, the method lists `X` in
that method's `throws` clause:
throws ..., X, ...
If a method chooses neither of those options, then that method won't inherit
exception documentation for `X` (assuming that exception documentation for `X`
exists in the supermethod).
Here's an informal, simplified, but conceptually correct version of the
algorithm that javadoc uses to document exceptions thrown by a method:
Step 1. For each exception tag, do the following. If an exception tag does
not have `{@inheritDoc}` in its description, add an entry for the exception
that this tag describes to the "Throws:" section. Otherwise, look for
corresponding documentation in the supermethod, to which apply this step (Step
1) recursively.
Step 2. For each exception from the `throws` clause, do the following. If an
exception has not been documented on the previous step, document it using
corresponding documentation in the supermethod, to which apply this algorithm
(both steps in order) recursively.
A few notes on the algorithm:
* Exception tags are examined prior to the `throws` clause. This allows a
method to **override** documentation that could be otherwise inherited from the
supermethod: if the method provides exception tags for a particular exception,
then documentation for that exception will be found on Step 1 and, hence, won't
be looked for in the supermethod on Step 2.
@throws X
* If a method wants to **add** documentation for a particular exception, rather
than **override** it, the method should both add documentation and inherit it
using tags:
@throws X
@throws X {@inheritDoc}
The above model might explain a common **misconception** according to which
methods inherit documentation for checked exceptions and do not inherit it for
unchecked exceptions. In reality, javadoc treats all exceptions equally. It's
just that if a method throws a checked exception, that exception (or its
superclass) must be listed in that method's `throws` clause. Now, if such an
exception is not documented by that method but documented by the supermethod,
that exception documentation is inherited. That gives a false impression that
the documentation is inherited because the exception is checked. In fact, the
documentation would still be inherited if that exception, listed in the
`throws` clause, were unchecked.
The above is how it has been working (barring bugs) since documentation comment
inheritance appeared in its current form. Implicit inheritance (filling in
missing text) appeared in JDK 1.3, explicit inheritance (`{@inheritDoc}`)
appeared in JDK 1.4, auto-inheriting documentation for subclasses of exceptions
whose documentation is inherited (JDK-4947455) appeared in JDK 5.
Back to this PR change in `java.desktop`. `ImageInputStreamImpl.readBoolean`
inherits `EOFException` documentation not because that method doesn't have a
doc comment of its own and, thus, inherits "the exact and entire" doc comment
from its supermethod (`ImageInputStream.readBoolean`). It's because that when
the algorithm reaches Step 2 and tries to find documentation for
Integrated: 8294377: Prepare to stop auto-inheriting documentation for subclasses of exceptions whose documentation is inherited
On Tue, 27 Sep 2022 11:43:08 GMT, Pavel Rappo wrote:
> This adds exception documentation to JDK methods that would otherwise lose
> that documentation once JDK-8287796 is integrated. While adding this
> exception documentation now does not change [^1] the JDK API Documentation,
> it will automatically counterbalance the effect that JDK-8287796 will have.
>
> JDK-8287796 seeks to remove an old, undocumented, and esoteric javadoc
> feature that cannot be suppressed [^2]. The feature is so little known about
> that IDEs either do not implement it correctly or do not implement it at all,
> thus rendering documentation differently from how the javadoc tool renders it.
>
> That feature was introduced in JDK-4947455 and manifests as follows. If a
> method inherits documentation for an exception, along with that documentation
> the method automatically inherits documentation for all exceptions that are
> subclasses of that former exception and are documented in an overridden
> method.
>
> To illustrate that behavior, consider the following example. A method
> `Subtype.m` inherits documentation for `SuperException`, while the overridden
> method `Supertype.m` documents `SuperException`, `SubExceptionA` and
> `SubExceptionB`, where the latter two exceptions are subclasses of the former
> exception:
>
> public class Subtype extends Supertype {
>
> @Override
> public void m() throws SuperException {
> ...
>
> public class Supertype {
>
> /**
> * @throws SuperException general problem
> * @throws SubExceptionA specific problem A
> * @throws SubExceptionB specific problem B
> */
> public void m() throws SuperException {
> ...
>
> public class SuperException extends Exception {
> ...
>
> public class SubExceptionA extends SuperException {
> ...
>
> public class SubExceptionB extends SuperException {
> ...
>
> For the above example, the API Documentation for `Subtype.m` will contain
> documentation for all three exceptions; the page fragment for `Subtype.m` in
> "Method Details" will look like this:
>
> public void m()
>throws SuperException
>
> Overrides:
> m in class Supertype
> Throws:
> SuperException - general problem
> SubExceptionA - specific problem A
> SubExceptionB - specific problem B
>
> It works for checked and unchecked exceptions, for methods in classes and
> interfaces.
>
>
> If it's the first time you hear about that behavior and this change affects
> your area, it's a good opportunity to reflect on whether the exception
> documentation this change adds is really needed or you were simply unaware of
> the fact that it was automatically added before. If exception documentation
> is not needed, please comment on this PR. Otherwise, consider approving it.
>
> Keep in mind that if some exception documentation is not needed, **leaving it
> out** from this PR might require a CSR.
>
>
> [^1]: Aside from insignificant changes on class-use and index pages. There's
> one relatively significant change. This change is in jdk.jshell, where some
> methods disappeared from "Methods declared in ..." section and other
> irregularities. We are aware of this and have filed JDK-8291803 to fix the
> root cause.
> [^2]: In reality, the feature can be suppressed, but it looks like a bug
> rather than intentional design. If we legitimize the feature and its
> suppression behavior, the model for exception documentation inheritance might
> become much more complicated.
This pull request has now been integrated.
Changeset: eb90c4fc
Author:Pavel Rappo
URL:
https://git.openjdk.org/jdk/commit/eb90c4fc0479379c8c1452afca8f37746c762e18
Stats: 320 lines in 14 files changed: 310 ins; 0 del; 10 mod
8294377: Prepare to stop auto-inheriting documentation for subclasses of
exceptions whose documentation is inherited
Reviewed-by: jjg
-
PR: https://git.openjdk.org/jdk/pull/10449
RFR: 8295168: Remove superfluous period in @throws tag description
Please review this utmost trivial fix for an issue discovered while working on
something else in jdk.javadoc. As far as I can see, this is the only case of
`{@inheritDoc}` not being the sole content of a `@throws` description in JDK.
-
Commit messages:
- Initial commit
Changes: https://git.openjdk.org/jdk/pull/10664/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=10664&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8295168
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/10664.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/10664/head:pull/10664
PR: https://git.openjdk.org/jdk/pull/10664
Integrated: 8295168: Remove superfluous period in @throws tag description
On Tue, 11 Oct 2022 17:11:49 GMT, Pavel Rappo wrote:
> Please review this utmost trivial fix for an issue discovered while working
> on something else in jdk.javadoc. As far as I can see, this is the only case
> of `{@inheritDoc}` not being the sole content of a `@throws` description in
> JDK.
This pull request has now been integrated.
Changeset: 3a980b97
Author: Pavel Rappo
URL:
https://git.openjdk.org/jdk/commit/3a980b972f72b5bbfd7bb63b433ae562890dbcf2
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
8295168: Remove superfluous period in @throws tag description
Reviewed-by: bpb, naoto, lancea, iris
-
PR: https://git.openjdk.org/jdk/pull/10664
Re: RFR: JDK-8296743: Tighten Class.getModifiers spec for array classes [v3]
On Fri, 18 Nov 2022 18:12:56 GMT, Joe Darcy wrote: >> Update the spec of Class.getModifiers to match long-standing behavior for >> primitive and array classes. Remove unneeded implementation flexibility with >> regard to setting other bit positions. This work was prompted to better >> support anticipated future Valhalla changes. >> >> Please also review the CSR: https://bugs.openjdk.org/browse/JDK-8297237 > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to review feedback. Feeling stupid for asking this, but does it make any sense to keep the order of modifiers "blessed" in text and tests? public protected private abstract static final transient volatile synchronized native strictfp - PR: https://git.openjdk.org/jdk/pull/11229
Re: RFR: JDK-8297298 - SequenceInputStream should override transferTo
On Sat, 19 Nov 2022 16:36:28 GMT, Markus KARG wrote: > Since JDK 18 some implementations of InputStream.transferTo, namely > FileInputStream and ChannelInputStream, offload work to lower layers using > NIO channels. This provides shorter execution time and reduced resource > consumption. Unfortunately, this effect is prevented once the input stream > itself is wrapped by a SequenceInputStream. While compared to other > InputStreams the SequenceInputStream is a rather edge case (e. g. used to > merge two files into one), nevertheless it makes sense to get rid of this > obstacle simply by implementing transferTo (e. g. by allowing to offload the > file merge, or parts of the file merge, to the operating system). As a > result, more existing applications will experience the > offloading-improvements made by JDK 18. > > To provide enhanced performance to existing applications, it makes sense to > address this impediment: SequenceInputStream.transferTo should be implemented > in a way that iteratively calls transferTo on each enumerated InputStream of > the SequenceInputStream in ordered sequence. I might be mistaken, but that vector is not temporary: the vector and the enumeration produced from it seems to be reachable for as long as the stream is. Both the enumeration and the vector are synchronized. That synchronization might provide implicit (JMM) visibility for some stream users. - PR: https://git.openjdk.org/jdk/pull/11248
Re: RFR: JDK-8297298 - SequenceInputStream should override transferTo
On Sun, 20 Nov 2022 07:30:15 GMT, Alan Bateman wrote: > > I might be mistaken, but that vector is not temporary: the vector and the > > enumeration produced from it seems to be reachable for as long as the > > stream is. Both the enumeration and the vector are synchronized. That > > synchronization might provide implicit (JMM) visibility for some stream > > users. > > I suspect you meant this comment for PR11249 rather than this one. Correct and sorry for the noise. - PR: https://git.openjdk.org/jdk/pull/11248
Re: RFR: 8299444 java.util.Set.copyOf allocates needlessly for empty input collections
On Wed, 4 Jan 2023 14:41:20 GMT, Viktor Klang wrote: > Currently Set.copyOf allocates both a HashSet and a new empty array when the > input collection is empty. > > This patch avoids allocating anything for the case where the parameter > collection's isEmpty returns true. Curious: how bad was that "needless allocation" that it was required to be optimized away? - PR: https://git.openjdk.org/jdk/pull/11847
Re: RFR: 8299852: Modernize ConcurrentHashMap
On Tue, 10 Jan 2023 10:32:34 GMT, Per Minborg wrote: > `java.util.concurrent.ConcurrentHashMap` is relatively old and has not been > updated to reflect the current state of Java and can be modernized: > > * Add `@Serial` annotations > * Seal classes and restrict subclassing for internal classes > * Use pattern matching for instance > * Remove redundant modifiers (such as some final declarations) > * Use Objects.requireNonNull for invariant checking > * Use diamond operators instead of explicit types > * Simplify expressions using Math::max I'm sure others will note or have already noted elsewhere that we don't do direct PRs against JSR 166. - PR: https://git.openjdk.org/jdk/pull/11924
Re: [jdk20] RFR: 8300719: JDK 20 RDP2 L10n resource files update [v5]
On Wed, 25 Jan 2023 18:00:33 GMT, Weijun Wang wrote: >> The translation tool didn't seem to translate this. Either because it >> couldn't or because it somehow missed it. I'm not sure which, but I'm open >> to replacing this with a translation suggestion you have. Or I can leave it >> as is. > > I'm not sure either. You can ask a javadoc expert whether this is a proper > noun or just plain English. I noticed it's also not translated in the > Japanese version but the German version has translated it. It's not a noun. It's an adjective that I had to synthesize for extra clarity and closeness to Java Language Specification (JLS). The English version of that entry is as follows: doclet.throwsInheritDocUnsupported=@inheritDoc is not supported for exception-type type parameters \ that are not declared by a method; document such exception types directly JLS _8.4.6. Method Throws_ defines BNF which conveniently labels elements, a list of which may appear in the `throws` clause, as `ExceptionType`. To make it more English-like and separate two otherwise consecutive occurrences of "type" in that sentence, I split the words with a hyphen and lower-cased them: exception-type. @jonathan-gibbons thoughts? - PR: https://git.openjdk.org/jdk20/pull/116
RFR: 8302981: Fix a typo in the doc comment for java.lang.Record.equals
Please review this trivial fix. - Commit messages: - Initial commit Changes: https://git.openjdk.org/jdk/pull/12689/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12689&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8302981 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/12689.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12689/head:pull/12689 PR: https://git.openjdk.org/jdk/pull/12689
Integrated: 8302981: Fix a typo in the doc comment for java.lang.Record.equals
On Tue, 21 Feb 2023 12:09:51 GMT, Pavel Rappo wrote: > Please review this trivial fix. This pull request has now been integrated. Changeset: 8b20aa91 Author: Pavel Rappo URL: https://git.openjdk.org/jdk/commit/8b20aa919b810fc5b3856b392bd0d8b1f882c895 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod 8302981: Fix a typo in the doc comment for java.lang.Record.equals Reviewed-by: jpai - PR: https://git.openjdk.org/jdk/pull/12689
Re: RFR: 8302815 Use new Math.clamp method in core libraries [v3]
On Tue, 21 Feb 2023 20:39:53 GMT, Tagir F. Valeev wrote:
>> For cleanup and dogfooding the new method, it would be nice to use
>> Math.clamp where possible in java.base. See PR #12428.
>>
>> As Math.clamp performs an additional check that min is not greater than max,
>> I conservatively replaced only those occurrences where I can see that this
>> invariant is always held. There are more occurrences, where clamp can be
>> potentially used but it's unclear whether min <= max is always true.
>
> Tagir F. Valeev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Update copyright year
I only saw this PR after it has been integrated. A code location that
immediately came to mind but was missing in the change is this
java/util/concurrent/SubmissionPublisher.java:1273:
public final void request(long n) {
if (n > 0L) {
for (;;) {
long p = demand, d = p + n; // saturate
if (casDemand(p, d < p ? Long.MAX_VALUE : d))
break;
}
startOnSignal(RUN | ACTIVE | REQS);
}
else
onError(new IllegalArgumentException(
"non-positive subscription request"));
}
Seems like a poster child for the new Math.clamp functionality.
-
PR: https://git.openjdk.org/jdk/pull/12633
Re: RFR: 8302815 Use new Math.clamp method in core libraries [v3]
On Tue, 21 Feb 2023 20:39:53 GMT, Tagir F. Valeev wrote: >> For cleanup and dogfooding the new method, it would be nice to use >> Math.clamp where possible in java.base. See PR #12428. >> >> As Math.clamp performs an additional check that min is not greater than max, >> I conservatively replaced only those occurrences where I can see that this >> invariant is always held. There are more occurrences, where clamp can be >> potentially used but it's unclear whether min <= max is always true. > > Tagir F. Valeev has updated the pull request incrementally with one > additional commit since the last revision: > > Update copyright year On second thought, maybe not; Math.clamp might actually look more clumsy here. Scratch my previous comment. - PR: https://git.openjdk.org/jdk/pull/12633
RFR: 8303350: Fix mistyped {@code}
Please review this trivial fix. - Commit messages: - Initial commit Changes: https://git.openjdk.org/jdk/pull/12784/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12784&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8303350 Stats: 6 lines in 3 files changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/12784.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12784/head:pull/12784 PR: https://git.openjdk.org/jdk/pull/12784
Integrated: 8303350: Fix mistyped {@code}
On Tue, 28 Feb 2023 13:31:06 GMT, Pavel Rappo wrote:
> Please review this trivial fix.
This pull request has now been integrated.
Changeset: dc5ea6ae
Author: Pavel Rappo
URL:
https://git.openjdk.org/jdk/commit/dc5ea6aeb500d531b4ba49c8e95bf97744cc6c33
Stats: 6 lines in 3 files changed: 0 ins; 0 del; 6 mod
8303350: Fix mistyped {@code}
Reviewed-by: jpai
-
PR: https://git.openjdk.org/jdk/pull/12784
RFR: 8303473: Add implied {@code} in java.lang.invoke.MethodHandles
Please review this trivial fix for _comments_. While some affected comments aren't doc comments and none of the affected comments partake in the API Documentation, it seems reasonable to be consistent. - Commit messages: - Initial commit Changes: https://git.openjdk.org/jdk/pull/12811/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12811&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8303473 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/12811.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12811/head:pull/12811 PR: https://git.openjdk.org/jdk/pull/12811
Re: RFR: 8303473: Add implied {@code} in java.lang.invoke.MethodHandles [v2]
> Please review this trivial fix for _comments_. While some affected comments
> aren't doc comments and none of the affected comments partake in the API
> Documentation, it seems reasonable to be consistent.
Pavel Rappo has updated the pull request incrementally with one additional
commit since the last revision:
fix!: mistyped {@code}
-
Changes:
- all: https://git.openjdk.org/jdk/pull/12811/files
- new: https://git.openjdk.org/jdk/pull/12811/files/656177c4..17d9c07d
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=12811&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=12811&range=00-01
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/12811.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/12811/head:pull/12811
PR: https://git.openjdk.org/jdk/pull/12811
Re: RFR: 8303473: Add implied {@code} in java.lang.invoke.MethodHandles
On Wed, 1 Mar 2023 18:47:14 GMT, Pavel Rappo wrote: > Please review this trivial fix for _comments_. While some affected comments > aren't doc comments and none of the affected comments partake in the API > Documentation, it seems reasonable to be consistent. Inadvertently found one more typo and fixed it in 17d9c07. Unlike other typos in this PR, that one crept into JDK Documentation. Please (re)review this PR. - PR: https://git.openjdk.org/jdk/pull/12811
RFR: 8303480: Miscellaneous fixes to mostly invisible doc comments
Please review this superficial documentation cleanup that was triggered by unrelated analysis of doc comments in JDK API. The only effect that this multi-area PR has on the JDK API Documentation (i.e. the observable effect on the generated HTML pages) can be summarized as follows: diff -ur build/macosx-aarch64/images/docs-before/api/serialized-form.html build/macosx-aarch64/images/docs-after/api/serialized-form.html --- build/macosx-aarch64/images/docs-before/api/serialized-form.html 2023-03-02 11:47:44 +++ build/macosx-aarch64/images/docs-after/api/serialized-form.html 2023-03-02 11:48:45 @@ -17084,7 +17084,7 @@ throws IOException, ClassNotFoundException readObject is called to restore the state of the - (@code BasicPermission} from a stream. + BasicPermission from a stream. Parameters: s - the ObjectInputStream from which data is read Notes - * I'm not an expert in any of the affected areas, except for jdk.javadoc, and I was merely after misused tags. Because of that, I would appreciate reviews from experts in other areas. * I discovered many more issues than I included in this PR. The excluded issues seem to occur in infrequently updated third-party code (e.g. javax.xml), which I assume we shouldn't touch unless necessary. * I will update copyright years after (and if) the fix had been approved, as required. - Commit messages: - Initial commit Changes: https://git.openjdk.org/jdk/pull/12826/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12826&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8303480 Stats: 75 lines in 39 files changed: 0 ins; 0 del; 75 mod Patch: https://git.openjdk.org/jdk/pull/12826.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12826/head:pull/12826 PR: https://git.openjdk.org/jdk/pull/12826
Re: RFR: JDK-8297605 DelayQueue javadoc is confusing [v2]
On Thu, 2 Mar 2023 11:17:10 GMT, Viktor Klang wrote: > Perhaps @pavelrappo has any suggestion for adding clarifying Javadoc to a > subclass without having to override the method? 🤔 You cannot do that. - PR: https://git.openjdk.org/jdk/pull/12729
Integrated: 8303473: Add implied {@code} in java.lang.invoke.MethodHandles
On Wed, 1 Mar 2023 18:47:14 GMT, Pavel Rappo wrote:
> Please review this trivial fix for _comments_. While some affected comments
> aren't doc comments and none of the affected comments partake in the API
> Documentation, it seems reasonable to be consistent.
This pull request has now been integrated.
Changeset: e1745bc9
Author: Pavel Rappo
URL:
https://git.openjdk.org/jdk/commit/e1745bc98180e0d49ed4dd3116a43c90645a1a09
Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod
8303473: Add implied {@code} in java.lang.invoke.MethodHandles
Reviewed-by: jjg, lancea, mchung
-
PR: https://git.openjdk.org/jdk/pull/12811
Re: RFR: 8303480: Miscellaneous fixes to mostly invisible doc comments
On Thu, 2 Mar 2023 16:23:17 GMT, Alexey Ivanov wrote:
>> Please review this superficial documentation cleanup that was triggered by
>> unrelated analysis of doc comments in JDK API.
>>
>> The only effect that this multi-area PR has on the JDK API Documentation
>> (i.e. the observable effect on the generated HTML pages) can be summarized
>> as follows:
>>
>>
>> diff -ur
>> build/macosx-aarch64/images/docs-before/api/serialized-form.html
>> build/macosx-aarch64/images/docs-after/api/serialized-form.html
>> --- build/macosx-aarch64/images/docs-before/api/serialized-form.html
>> 2023-03-02 11:47:44
>> +++ build/macosx-aarch64/images/docs-after/api/serialized-form.html
>> 2023-03-02 11:48:45
>> @@ -17084,7 +17084,7 @@
>> throws > href="java.base/java/io/IOException.html" title="class in
>> java.io">IOException,
>> ClassNotFoundException
>> readObject is called to restore the
>> state of the
>> - (@code BasicPermission} from a stream.
>> + BasicPermission from a stream.
>>
>> Parameters:
>> s - the ObjectInputStream from which data
>> is read
>>
>> Notes
>> -
>>
>> * I'm not an expert in any of the affected areas, except for jdk.javadoc,
>> and I was merely after misused tags. Because of that, I would appreciate
>> reviews from experts in other areas.
>> * I discovered many more issues than I included in this PR. The excluded
>> issues seem to occur in infrequently updated third-party code (e.g.
>> javax.xml), which I assume we shouldn't touch unless necessary.
>> * I will update copyright years after (and if) the fix had been approved, as
>> required.
>
> src/java.base/share/classes/java/lang/invoke/BootstrapMethodInvoker.java line
> 257:
>
>> 255:
>> 256: /**
>> 257: * @return true iff the BSM method type exactly matches
>
> I assume “iff” should “if”?
Here and elsewhere in this file "iff" might mean [if and only
if](https://en.wikipedia.org/wiki/If_and_only_if), which would make sense.
(FWIW, there are a few hundred occurrences of the word "iff" in src.)
@cl4es (Claes Redestad), as the author of those lines would you like to chime
in?
Since Claes might read this, I note that when I changed unsupported `{@see}` to
`{@link}` thoughtout this file, my IDE could not resolve one of the links:
`java.lang.invoke.LambdaMetafactory#metafactory(MethodHandles.Lookup,String,Class,MethodType,MethodHandle,MethodType)`
While there's a similarly-name method with slightly different parameters, I
refrained from using it:
`java.lang.invoke.LambdaMetafactory#metafactory(MethodHandles.Lookup,String,MethodType,MethodType,MethodHandle,MethodType)`.
-
PR: https://git.openjdk.org/jdk/pull/12826
Re: RFR: 8303480: Miscellaneous fixes to mostly invisible doc comments
On Fri, 3 Mar 2023 08:15:49 GMT, Alexey Ivanov wrote:
>> Please review this superficial documentation cleanup that was triggered by
>> unrelated analysis of doc comments in JDK API.
>>
>> The only effect that this multi-area PR has on the JDK API Documentation
>> (i.e. the observable effect on the generated HTML pages) can be summarized
>> as follows:
>>
>>
>> diff -ur
>> build/macosx-aarch64/images/docs-before/api/serialized-form.html
>> build/macosx-aarch64/images/docs-after/api/serialized-form.html
>> --- build/macosx-aarch64/images/docs-before/api/serialized-form.html
>> 2023-03-02 11:47:44
>> +++ build/macosx-aarch64/images/docs-after/api/serialized-form.html
>> 2023-03-02 11:48:45
>> @@ -17084,7 +17084,7 @@
>> throws > href="java.base/java/io/IOException.html" title="class in
>> java.io">IOException,
>> ClassNotFoundException
>> readObject is called to restore the
>> state of the
>> - (@code BasicPermission} from a stream.
>> + BasicPermission from a stream.
>>
>> Parameters:
>> s - the ObjectInputStream from which data
>> is read
>>
>> Notes
>> -
>>
>> * I'm not an expert in any of the affected areas, except for jdk.javadoc,
>> and I was merely after misused tags. Because of that, I would appreciate
>> reviews from experts in other areas.
>> * I discovered many more issues than I included in this PR. The excluded
>> issues seem to occur in infrequently updated third-party code (e.g.
>> javax.xml), which I assume we shouldn't touch unless necessary.
>> * I will update copyright years after (and if) the fix had been approved, as
>> required.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java line 2866:
>
>> 2864: * Merge multiple abstract methods. The preferred method is a
>> method that is a subsignature
>> 2865: * of all the other signatures and whose return type is more
>> specific {@link MostSpecificReturnCheck}.
>> 2866: * The resulting preferred method has a thrown clause that is the
>> intersection of the merged
>
> Is it “…has a {@code throws} clause…”?
Thanks! I'll add this to a separate PR.
-
PR: https://git.openjdk.org/jdk/pull/12826
Re: RFR: 8303480: Miscellaneous fixes to mostly invisible doc comments [v2]
> Please review this superficial documentation cleanup that was triggered by > unrelated analysis of doc comments in JDK API. > > The only effect that this multi-area PR has on the JDK API Documentation > (i.e. the observable effect on the generated HTML pages) can be summarized as > follows: > > > diff -ur build/macosx-aarch64/images/docs-before/api/serialized-form.html > build/macosx-aarch64/images/docs-after/api/serialized-form.html > --- build/macosx-aarch64/images/docs-before/api/serialized-form.html > 2023-03-02 11:47:44 > +++ build/macosx-aarch64/images/docs-after/api/serialized-form.html > 2023-03-02 11:48:45 > @@ -17084,7 +17084,7 @@ > throws href="java.base/java/io/IOException.html" title="class in > java.io">IOException, > ClassNotFoundException > readObject is called to restore the > state of the > - (@code BasicPermission} from a stream. > + BasicPermission from a stream. > > Parameters: > s - the ObjectInputStream from which data > is read > > Notes > - > > * I'm not an expert in any of the affected areas, except for jdk.javadoc, and > I was merely after misused tags. Because of that, I would appreciate reviews > from experts in other areas. > * I discovered many more issues than I included in this PR. The excluded > issues seem to occur in infrequently updated third-party code (e.g. > javax.xml), which I assume we shouldn't touch unless necessary. > * I will update copyright years after (and if) the fix had been approved, as > required. Pavel Rappo has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision: - Merge branch 'master' into 8303480 - Initial commit - Changes: - all: https://git.openjdk.org/jdk/pull/12826/files - new: https://git.openjdk.org/jdk/pull/12826/files/d2f4a553..87166408 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12826&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12826&range=00-01 Stats: 13433 lines in 415 files changed: 9003 ins; 2610 del; 1820 mod Patch: https://git.openjdk.org/jdk/pull/12826.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12826/head:pull/12826 PR: https://git.openjdk.org/jdk/pull/12826
Integrated: 8303480: Miscellaneous fixes to mostly invisible doc comments
On Thu, 2 Mar 2023 12:03:44 GMT, Pavel Rappo wrote: > Please review this superficial documentation cleanup that was triggered by > unrelated analysis of doc comments in JDK API. > > The only effect that this multi-area PR has on the JDK API Documentation > (i.e. the observable effect on the generated HTML pages) can be summarized as > follows: > > > diff -ur build/macosx-aarch64/images/docs-before/api/serialized-form.html > build/macosx-aarch64/images/docs-after/api/serialized-form.html > --- build/macosx-aarch64/images/docs-before/api/serialized-form.html > 2023-03-02 11:47:44 > +++ build/macosx-aarch64/images/docs-after/api/serialized-form.html > 2023-03-02 11:48:45 > @@ -17084,7 +17084,7 @@ > throws href="java.base/java/io/IOException.html" title="class in > java.io">IOException, > ClassNotFoundException > readObject is called to restore the > state of the > - (@code BasicPermission} from a stream. > + BasicPermission from a stream. > > Parameters: > s - the ObjectInputStream from which data > is read > > Notes > - > > * I'm not an expert in any of the affected areas, except for jdk.javadoc, and > I was merely after misused tags. Because of that, I would appreciate reviews > from experts in other areas. > * I discovered many more issues than I included in this PR. The excluded > issues seem to occur in infrequently updated third-party code (e.g. > javax.xml), which I assume we shouldn't touch unless necessary. > * I will update copyright years after (and if) the fix had been approved, as > required. This pull request has now been integrated. Changeset: 45a616a8 Author:Pavel Rappo URL: https://git.openjdk.org/jdk/commit/45a616a891e4a4b0e77b1f2fa040522f4a99d172 Stats: 75 lines in 39 files changed: 0 ins; 0 del; 75 mod 8303480: Miscellaneous fixes to mostly invisible doc comments Reviewed-by: mullan, prr, cjplummer, aivanov, jjg, lancea, rriggs, ihse - PR: https://git.openjdk.org/jdk/pull/12826
Re: RFR: JDK-8303742: CompletableFuture.orTimeout leaks if the future completes exceptionally [v2]
On Thu, 16 Mar 2023 20:51:37 GMT, Viktor Klang wrote:
>> Addresses the situation where exceptional completion of `orTimeout`:ed
>> CompletableFutures wouldn't cancel the timeout task which could lead to
>> memory leaks if done frequently enough with long enough timeout durations.
>>
>> Fix discussed with @DougLea
>
> Viktor Klang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update
> test/jdk/java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java
>
> Co-authored-by: Andrey Turbanov
test/jdk/java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java
line 44:
> 42: void testOrTimeoutWithCompleteExceptionallyDoesNotLeak() throws
> Exception {
> 43: var startTime = System.currentTimeMillis();
> 44: var testRunTime = Duration.ofSeconds(10).toMillis();
This "create completable futures in a loop for t seconds" seems a bit brittle.
Would 10 or 20 seconds be enough for a typical test machine to fail with OOME?
Could this be improved by requiring a minimum number of CF instances to be
created? Maybe finishing when t seconds have elapsed _and_ n instances have
been created.
Separately, as with any timeouts, consider a monotonic clock.
-
PR: https://git.openjdk.org/jdk/pull/13059
Re: RFR: JDK-8303742: CompletableFuture.orTimeout leaks if the future completes exceptionally [v3]
On Thu, 16 Mar 2023 21:21:51 GMT, Viktor Klang wrote: >> Addresses the situation where exceptional completion of `orTimeout`:ed >> CompletableFutures wouldn't cancel the timeout task which could lead to >> memory leaks if done frequently enough with long enough timeout durations. >> >> Fix discussed with @DougLea > > Viktor Klang has updated the pull request incrementally with one additional > commit since the last revision: > > Making the test for orTimeout+completeExceptionally only based on > iterations and not duration. test/jdk/java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java line 2: > 1: /* > 2: * Copyright (c) 2019, 2023, Oracle and/or its affiliates. All rights > reserved. 2019? - PR: https://git.openjdk.org/jdk/pull/13059
Re: RFR: 8302818: Optimize wrapper sets and immutable sets of Enums [v5]
On Tue, 21 Mar 2023 15:23:53 GMT, Claes Redestad wrote: > An alternative design which would avoid adding more classes could be to add > package-private accessors to the existing `Unmodifiable/Synchronized` wrapper > classes so that `EnumSet/-Map` can retrieve the underlying set of an > unmodifiable or synchronized `Set` or `Map` and then use it directly for > these bulk operations. Then you'd contain the additional overhead to > `EnumSet/-Map`. Another alternative, in cases where `arg.getClass() == X.class` can be safely substituted with `requireNonNull(arg) instanceof X`, is a marker interface. - PR Comment: https://git.openjdk.org/jdk/pull/12498#issuecomment-1478253385
Re: RFR: JDK-8304557: java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java times out
On Wed, 22 Mar 2023 11:21:14 GMT, Viktor Klang wrote: >> Hello Viktor, >> >>> @jaikiran Having a long timeout doesn't seem like a problem given that it >>> just needs enough time to run through the iterations (i.e. that's the max >>> duration of the test before giving up). >> >> Having a extremely long timeout of T1 secods for a test which one can >> (reasonably) expect to finish in (T1 - X) seconds can mean that when/if it >> genuinely times out, then it holds on to the limited shared resources (like >> the host machine) for those X seconds longer instead of potentially letting >> other tests run during that period. So I think it's always better to have a >> reasonable timeout instead of an extremely large one - a value past which >> you know that if the test is still running then it's surely be a sign that >> it should no longer continue to run. >> Typically the timeout for such tests is decided by running the test against >> the hosts/environment where it failed and gathering data to see how long it >> usually takes to finish successfully on those and then adding some extra >> seconds to it. > > @jaikiran The downside is that it is hard to judge how long it will take for > machine X to execute Y instructions. :/ 1. Is there a way to mark this test as skipped if the timeout elapses? I suggest we don't mark it as skipped based on the build or any other configuration, but actaully run it and see if it fails or succeeds. The rationale is this: if the test is still running when the time is out, it means that the test is inconclusive. 2. Can we reduce memory configuration to conserve system resources? Say, set `-Xmx` to 96 or even 64 megabytes. Or is so little / unusual that we'll risk getting other issues on modern JVMs? 3. I don't see why we need to reduce the number of iterations. What we should do instead is compute it, otherwise that number, whatever it is, looks magic. There needs to be some rationale behing that number, even if that rationale is not accurate. Say, we can reasonably assume that overhead for one `CompletableFuture` is at least one byte. So, to fill up `N` megabyte(s) of heap we would need `N * 1_048_576` iterations. - PR Review Comment: https://git.openjdk.org/jdk/pull/13116#discussion_r114427
Re: RFR: JDK-8304557: java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java times out
On Wed, 22 Mar 2023 16:06:51 GMT, Viktor Klang wrote: > The other alternative I see would be to reach into the implementation of > CompletableFuture's `Delayer`'s `ScheduledThreadPoolExecutor delayer` and > make sure that it's `getQueue()` eventually goes empty. >From what I've seen, JDK prefers blackbox tests to whitebox tests. Even though >the latter might conserve resources better and are easier to implement, they >are problematic in the long run: they become an obstacle if the system under >tests is modified. - PR Review Comment: https://git.openjdk.org/jdk/pull/13116#discussion_r1145108257
Re: RFR: JDK-8304557: java/util/concurrent/CompletableFuture/CompletableFutureOrTimeoutExceptionallyTest.java times out [v2]
On Thu, 23 Mar 2023 12:04:34 GMT, Alan Bateman wrote: >> If there isn't any value running this test with a debug build then you could >> have it skip (`@requires vm.debug == false`) to save resources. >> (fixed typo in original message) > >> @AlanBateman @pavelrappo Does the new approach seem acceptable? > > I don't have a strong opinion on this. The advantage is that the test is > probably sub-second as it just needs one CF to complete exceptionally. The > disadvantage is that it the test would need to be updated if the > implementation changes, but it's not hard. > > So good with the approach if you want. As Jai mentioned, you can use @modules > to open java.base/java.util.concurrent for deep reflectively. I would > probably trim the overly long lines as they currently wrap when looking at > the diffs. While the approach seems okay, it might be for the JSR 166 folk to make the final decision on it. (Looking at Thread.sleep(). Sigh: unfortunately, BlockingQueue does not provide an option to examine its head that blocks or times out.) - PR Review Comment: https://git.openjdk.org/jdk/pull/13116#discussion_r1146118762
Re: JDK 20 EA builds (archive?)
Hi Chris, Would you not be able to bisect by tags? Checkout, build, and run your test(s) possibly (semi-) automatically using git bisect? What's the OS? -Pavel > On 24 Mar 2023, at 16:55, Chris Hegarty wrote: > > Hi, > > While definitely not the right list, someone here will surely know ... > > Where can I download archive EA builds of JDK 20, so I can perform bisect > experiments to help narrow when something changed between JDK 19 and JDK 20. > ( I have some builds, but not that many ) > > --- > Informational: I'm not expecting anyone to do my work for me ;-) but if you > have ideas or hints! > > I'm try to track down a change in G1 GC behaviour, where the number of GC's > of the Young generation has decreased ( by ~20% ), but the cumulative total > time spent in GC of the Young generation has increased marginally ( ~2% ) for > a particular workload benchmark ( that runs for a medium amount of time, ~20 > mins ). > > Thanks, > -Chris.
Re: RFR: 8304919: Implementation of Virtual Threads [v2]
On Tue, 28 Mar 2023 23:47:02 GMT, Leonid Mesnik wrote:
>> Alan Bateman has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains four additional
>> commits since the last revision:
>>
>> - ThreadSleepEvent refactoring
>> - Merge
>> - Merge
>> - Initial sync from fibers branch
>
> src/java.base/share/classes/java/lang/System.java line 2566:
>
>> 2564:
>> 2565: public V executeOnCarrierThread(Callable task)
>> throws Exception {
>> 2566: if (Thread.currentThread() instanceof VirtualThread
>> vthread) {
>
> Any specific reason to don't use Thread.currentThread().isVirtual()?
To use the pattern variable to call `executeOnCarrierThread` on it?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13203#discussion_r1151270089
Re: RFR: 8305157: The java.util.Arrays class should be declared final
On Wed, 29 Mar 2023 09:03:58 GMT, Alan Bateman wrote: > > I thought the change would trigger a change in the public API as the > > modifiers are changed for the class. This would likely be picked up by > > compatibility checks and so, a CSR would be needed? > > It's not a compatibility issue but a CSR is needed to track the change to the > signature. Not that I disagree with this PR, but it would be interesting to see rationale for it in CSR. - PR Comment: https://git.openjdk.org/jdk/pull/13221#issuecomment-1488804341
Re: RFR: 8306075: Micro-optimize Enum.hashCode
On Mon, 17 Apr 2023 10:59:34 GMT, olivergillespie wrote:
> Improve the speed of Enum.hashCode by caching the identity hashcode on first
> use. I've seen an application where Enum.hashCode is a hot path, and this is
> fairly simple speedup. The memory overhead is low; in enums with no extra
> fields there is already a 4-byte space due to alignment so this new field can
> slot in 'for free'. In other cases, the singleton nature of enum values means
> that the number of total instances is typically very low, so a small
> per-instance overhead is not a concern.
>
> Please see more discussion/explanation in the [original enhancement
> request](https://bugs.openjdk.org/browse/JDK-8306075).
>
> ### Benchmark
>
>
>
> Before:
>
> Benchmark Mode Cnt Score Error Units
> # Intel Cascade lake
> EnumHashCode.constant avgt 15 1.602 ± 0.011 ns/op
> EnumHashCode.field avgt 15 1.681 ± 0.014 ns/op
> # Arm Neoverse N1
> EnumHashCode.constant avgt 15 1.642 ± 0.033 ns/op
> EnumHashCode.field avgt 15 1.717 ± 0.059 ns/op
>
>
>
> After:
>
> Benchmark Mode Cnt Score Error Units
> # Intel Cascade lake
> EnumHashCode.constant avgt 15 0.479 ± 0.001 ns/op
> EnumHashCode.field avgt 15 0.799 ± 0.002 ns/op
> # Arm Neoverse N1
> EnumHashCode.constant avgt 15 0.802 ± 0.002 ns/op
> EnumHashCode.field avgt 15 1.059 ± 0.056 ns/op
>
>
> Using `-prof perfasm` on the benchmark, we can compare the generated code for
> x86_64:
>
> Before:
>
> │ 0x7fae4868dd17: lea(%r12,%r10,8),%rsi ;*getfield e
> {reexecute=0 rethrow=0 return_oop=0}
> │ ; -
> org.sample.EnumHashCode::field@1 (line 24)
> │ ; -
> org.sample.jmh_generated.EnumHashCode_field_jmhTest::field_avgt_jmhStub@17
> (line 186)
> │ 0x7fae4868dd1b: mov(%rsi),%r10
> │ 0x7fae4868dd1e: mov%r10,%r11
> │ 0x7fae4868dd21: and$0x3,%r11
> │ 0x7fae4868dd25: cmp$0x1,%r11
> │ 0x7fae4868dd29: jne0x7fae4868dcc6
> │ 0x7fae4868dd2b: shr$0x8,%r10
> │ 0x7fae4868dd2f: mov%r10d,%eax
> │ 0x7fae4868dd32: and$0x7fff,%eax
> │ 0x7fae4868dd37: test %eax,%eax
> │ 0x7fae4868dd39: je 0x7fae4868dcc6 ;*invokespecial
> hashCode {reexecute=0 rethrow=0 return_oop=0}
> │ ; -
> java.lang.Enum::hashCode@1 (line 175)
>
>
> This is the normal Object.hashCode intrinsic, which involves reading the
> object header, extracting the hash code and handling two slow-path cases
> (displaced object header, hash not initialized).
>
> After:
>
>
> │ 0x7f550068e3b4: mov0x10(%r12,%r10,8),%r8d <-- read the hash
> field
> │ 0x7f550068e3b9: test %r8d,%r8d <-- if (hash == 0)
> │ 0x7f550068e3bc: je 0x7f550068e413 <-- slow init path,
> only taken on first use
>
>
> Thanks @shipilev for help with the implementation and interpreting the
> generated code.
src/java.base/share/classes/java/lang/Enum.java line 175:
> 173: *
> 174: * @implNote Once initialized, the field value does not change.
> 175: * Hotspot's identity hash code generation also never returns zero
> Hotspot's identity hash code generation also never returns zero
Isn't that behavior VM-specific? Also, where is it documented?
src/java.base/share/classes/java/lang/Enum.java line 191:
> 189: int hc = hash;
> 190: if (hc == 0) {
> 191: hc = hash = System.identityHashCode(this);
Why not `hc = hash = super.hashCode()`?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13491#discussion_r1168593999
PR Review Comment: https://git.openjdk.org/jdk/pull/13491#discussion_r1168601440
Re: RFR: 8306075: Micro-optimize Enum.hashCode [v3]
On Mon, 17 Apr 2023 13:08:41 GMT, Aleksey Shipilev wrote: >> Yes, it is implementation-specific, that is why it says "Hotspot's identity >> hash code". The relevant code blocks are >> https://github.com/openjdk/jdk/blob/cc60f2ff3f16bdb04917e09cb87f09bd544f1f8b/src/hotspot/share/oops/markWord.hpp#L231-L233 >> (property) and >> https://github.com/openjdk/jdk/blob/cc60f2ff3f16bdb04917e09cb87f09bd544f1f8b/src/hotspot/share/runtime/synchronizer.cpp#L826-L827 >> (invariant). > > It would not break the code functionally if that invariant ever breaks: we > would "just" call the (intrinsic) method on zero hash code. That `implNote` > only shows that it would not happen with current implementation at all. >From that impl note it seemed like it was a big deal for hash code to never >return 0 for an object. Could you maybe de-emphasize the importance of that >HotSpot behavior in the note? - PR Review Comment: https://git.openjdk.org/jdk/pull/13491#discussion_r1168725310
Re: RFR: 8306075: Micro-optimize Enum.hashCode [v3]
On Mon, 17 Apr 2023 13:27:43 GMT, olivergillespie wrote:
>> Improve the speed of Enum.hashCode by caching the identity hashcode on first
>> use. I've seen an application where Enum.hashCode is a hot path, and this is
>> fairly simple speedup. The memory overhead is low; in enums with no extra
>> fields there is already a 4-byte space due to alignment so this new field
>> can slot in 'for free'. In other cases, the singleton nature of enum values
>> means that the number of total instances is typically very low, so a small
>> per-instance overhead is not a concern.
>>
>> Please see more discussion/explanation in the [original enhancement
>> request](https://bugs.openjdk.org/browse/JDK-8306075).
>>
>> ### Benchmark
>>
>>
>>
>> Before:
>>
>> Benchmark Mode Cnt Score Error Units
>> # Intel Cascade lake
>> EnumHashCode.constant avgt 15 1.602 ± 0.011 ns/op
>> EnumHashCode.field avgt 15 1.681 ± 0.014 ns/op
>> # Arm Neoverse N1
>> EnumHashCode.constant avgt 15 1.642 ± 0.033 ns/op
>> EnumHashCode.field avgt 15 1.717 ± 0.059 ns/op
>>
>>
>>
>> After:
>>
>> Benchmark Mode Cnt Score Error Units
>> # Intel Cascade lake
>> EnumHashCode.constant avgt 15 0.479 ± 0.001 ns/op
>> EnumHashCode.field avgt 15 0.799 ± 0.002 ns/op
>> # Arm Neoverse N1
>> EnumHashCode.constant avgt 15 0.802 ± 0.002 ns/op
>> EnumHashCode.field avgt 15 1.059 ± 0.056 ns/op
>>
>>
>> Using `-prof perfasm` on the benchmark, we can compare the generated code
>> for x86_64:
>>
>> Before:
>>
>> │ 0x7fae4868dd17: lea(%r12,%r10,8),%rsi ;*getfield e
>> {reexecute=0 rethrow=0 return_oop=0}
>> │ ; -
>> org.sample.EnumHashCode::field@1 (line 24)
>> │ ; -
>> org.sample.jmh_generated.EnumHashCode_field_jmhTest::field_avgt_jmhStub@17
>> (line 186)
>> │ 0x7fae4868dd1b: mov(%rsi),%r10
>> │ 0x7fae4868dd1e: mov%r10,%r11
>> │ 0x7fae4868dd21: and$0x3,%r11
>> │ 0x7fae4868dd25: cmp$0x1,%r11
>> │ 0x7fae4868dd29: jne0x7fae4868dcc6
>> │ 0x7fae4868dd2b: shr$0x8,%r10
>> │ 0x7fae4868dd2f: mov%r10d,%eax
>> │ 0x7fae4868dd32: and$0x7fff,%eax
>> │ 0x7fae4868dd37: test %eax,%eax
>> │ 0x7fae4868dd39: je 0x7fae4868dcc6 ;*invokespecial
>> hashCode {reexecute=0 rethrow=0 return_oop=0}
>> │ ; -
>> java.lang.Enum::hashCode@1 (line 175)
>>
>>
>> This is the normal Object.hashCode intrinsic, which involves reading the
>> object header, extracting the hash code and handling two slow-path cases
>> (displaced object header, hash not initialized).
>>
>> After:
>>
>>
>> │ 0x7f550068e3b4: mov0x10(%r12,%r10,8),%r8d <-- read the hash
>> field
>> │ 0x7f550068e3b9: test %r8d,%r8d <-- if (hash == 0)
>> │ 0x7f550068e3bc: je 0x7f550068e413 <-- slow init
>> path, only taken on first use
>>
>>
>> Thanks @shipilev for help with the implementation and interpreting the
>> generated code.
>
> olivergillespie has refreshed the contents of this pull request, and previous
> commits have been removed. Incremental views are not available. The pull
> request now contains one commit:
>
> 8306075: Micro-optimize Enum.hashCode
src/java.base/share/classes/java/lang/Enum.java line 175:
> 173: *
> 174: * @implNote Once initialized, the field value does not change.
> 175: * Hotspot's identity hash code generation also never returns zero
Trivial: it's camel case in HotSpot
For example: https://en.wikipedia.org/wiki/HotSpot_(virtual_machine)
src/java.base/share/classes/java/lang/Enum.java line 177:
> 175: * Hotspot's identity hash code generation also never returns zero
> 176: * as the identity hash code. This allows to treat zero as the marker
> 177: * for the un-initialized value for both {#link @Stable} and the lazy
Typo: `{#link @Stable}`
I'd suggest using either `{@link Stable}` or `{@code @Stable}`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13491#discussion_r1168731449
PR Review Comment: https://git.openjdk.org/jdk/pull/13491#discussion_r1168734730
Re: RFR: 8306075: Micro-optimize Enum.hashCode [v4]
On Mon, 17 Apr 2023 14:15:39 GMT, olivergillespie wrote:
>> Improve the speed of Enum.hashCode by caching the identity hashcode on first
>> use. I've seen an application where Enum.hashCode is a hot path, and this is
>> fairly simple speedup. The memory overhead is low; in enums with no extra
>> fields there is already a 4-byte space due to alignment so this new field
>> can slot in 'for free'. In other cases, the singleton nature of enum values
>> means that the number of total instances is typically very low, so a small
>> per-instance overhead is not a concern.
>>
>> Please see more discussion/explanation in the [original enhancement
>> request](https://bugs.openjdk.org/browse/JDK-8306075).
>>
>> ### Benchmark
>>
>>
>>
>> Before:
>>
>> Benchmark Mode Cnt Score Error Units
>> # Intel Cascade lake
>> EnumHashCode.constant avgt 15 1.602 ± 0.011 ns/op
>> EnumHashCode.field avgt 15 1.681 ± 0.014 ns/op
>> # Arm Neoverse N1
>> EnumHashCode.constant avgt 15 1.642 ± 0.033 ns/op
>> EnumHashCode.field avgt 15 1.717 ± 0.059 ns/op
>>
>>
>>
>> After:
>>
>> Benchmark Mode Cnt Score Error Units
>> # Intel Cascade lake
>> EnumHashCode.constant avgt 15 0.479 ± 0.001 ns/op
>> EnumHashCode.field avgt 15 0.799 ± 0.002 ns/op
>> # Arm Neoverse N1
>> EnumHashCode.constant avgt 15 0.802 ± 0.002 ns/op
>> EnumHashCode.field avgt 15 1.059 ± 0.056 ns/op
>>
>>
>> Using `-prof perfasm` on the benchmark, we can compare the generated code
>> for x86_64:
>>
>> Before:
>>
>> │ 0x7fae4868dd17: lea(%r12,%r10,8),%rsi ;*getfield e
>> {reexecute=0 rethrow=0 return_oop=0}
>> │ ; -
>> org.sample.EnumHashCode::field@1 (line 24)
>> │ ; -
>> org.sample.jmh_generated.EnumHashCode_field_jmhTest::field_avgt_jmhStub@17
>> (line 186)
>> │ 0x7fae4868dd1b: mov(%rsi),%r10
>> │ 0x7fae4868dd1e: mov%r10,%r11
>> │ 0x7fae4868dd21: and$0x3,%r11
>> │ 0x7fae4868dd25: cmp$0x1,%r11
>> │ 0x7fae4868dd29: jne0x7fae4868dcc6
>> │ 0x7fae4868dd2b: shr$0x8,%r10
>> │ 0x7fae4868dd2f: mov%r10d,%eax
>> │ 0x7fae4868dd32: and$0x7fff,%eax
>> │ 0x7fae4868dd37: test %eax,%eax
>> │ 0x7fae4868dd39: je 0x7fae4868dcc6 ;*invokespecial
>> hashCode {reexecute=0 rethrow=0 return_oop=0}
>> │ ; -
>> java.lang.Enum::hashCode@1 (line 175)
>>
>>
>> This is the normal Object.hashCode intrinsic, which involves reading the
>> object header, extracting the hash code and handling two slow-path cases
>> (displaced object header, hash not initialized).
>>
>> After:
>>
>>
>> │ 0x7f550068e3b4: mov0x10(%r12,%r10,8),%r8d <-- read the hash
>> field
>> │ 0x7f550068e3b9: test %r8d,%r8d <-- if (hash == 0)
>> │ 0x7f550068e3bc: je 0x7f550068e413 <-- slow init
>> path, only taken on first use
>>
>>
>> Thanks @shipilev for help with the implementation and interpreting the
>> generated code.
>
> olivergillespie has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fix two typos
src/java.base/share/classes/java/lang/Enum.java line 177:
> 175: * HotSpot's identity hash code generation also never returns zero
> 176: * as the identity hash code. This makes zero a convenient marker
> 177: * for the un-initialized value for both {@link @Stable} and the lazy
I'm sorry, but `@Stable` won't work with `@link`. `@Stable` is not a class,
interface, member name, etc. See
https://docs.oracle.com/en/java/javase/20/docs/specs/javadoc/doc-comment-spec.html#references
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13491#discussion_r1168783477
Re: RFR: 8306075: Micro-optimize Enum.hashCode [v3]
On Mon, 17 Apr 2023 13:49:11 GMT, Aleksey Shipilev wrote: >> From that impl note it seemed like it was a big deal for hash code to never >> return 0 for an object. Could you maybe de-emphasize the importance of that >> HotSpot behavior in the note? > > All right, we can change "This allows to treat zero as the marker..." to > "This makes zero a convenient marker...", I think? To me, the proposed implementation looks similar to that of String::hashCode before Compact Strings (JEP 254). The interesting detail here is the fact that unlike String::hashCode, which can legitimately return 0, Object::hashCode cannot, at least in HotSpot. This is great news, because to be performant, java.lang.Enum does not need to be bloated with extra mechanics, such as that boolean `hashIsZero` introduced for java.lang.String in 8221836: Avoid recalculating String.hash when zero. If the essence of that can be compressed to a few simple sentences and possibly moved to an inline comment in Enum::hashCode, that _might_ be better. - PR Review Comment: https://git.openjdk.org/jdk/pull/13491#discussion_r1168853799
Re: RFR: 8306075: Micro-optimize Enum.hashCode [v4]
On Mon, 17 Apr 2023 14:56:13 GMT, Roger Riggs wrote: > The @implNote is more appropriate for an internal comment. It is not needed > to be in the published javadoc; its only useful to someone viewing the source > code. Is it a comment to me or to the author? If it's the former, then I note, that if one bothers to mark up a comment, that markup should be valid. - PR Review Comment: https://git.openjdk.org/jdk/pull/13491#discussion_r1168866435
Re: RFR: 8306075: Micro-optimize Enum.hashCode [v5]
On Mon, 17 Apr 2023 15:55:38 GMT, Aleksey Shipilev wrote: >> olivergillespie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Switch to non-javadoc comment and remove markup > > src/java.base/share/classes/java/lang/Enum.java line 171: > >> 169: } >> 170: >> 171: /* > > Should be `/**` (Javadoc style), like other `private` fields. I think the reviewers are fine with the essence of this PR. As for the comment, we could split it in two: the javadoc part, which stays with the field, and the inline part, which moves to the method. See String.hash and String::hashCode. - PR Review Comment: https://git.openjdk.org/jdk/pull/13491#discussion_r1168963655
Re: RFR: 8306075: Micro-optimize Enum.hashCode [v6]
On Mon, 17 Apr 2023 16:44:24 GMT, Aleksey Shipilev wrote: > I am okay with this new version, thanks. (You need other commenters to > approve as well.) While there's already one more reviewer who approved this PR (@cl4es) and technically it can be already integrated, I'd leave it for a day or two so that other Reviewers could see it too. - PR Comment: https://git.openjdk.org/jdk/pull/13491#issuecomment-1511731776
Re: RFR: 8266571: Sequenced Collections [v7]
On Wed, 19 Apr 2023 04:36:28 GMT, Chen Liang wrote:
> Javadoc will automatically copy the specification from the overridden method,
> so the javadoc block is not necessary and can be replaced by an `@Override`
> to indicate inheritance.
I guess, this PR has converged enough for us to discuss some trivial stuff; so
here it goes.
While you are right when saying that a doc comment consisting of a lone
`{@inheritDoc}` is -- and I'm paraphrasing here -- the same as no comment at
all, such a comment effectively is a copy of the overridden method _main
description_, which is the part of a doc comment from its beginning to the
first block tag or the end of the comment (if the comment has no block tags).
Parameters, return value and exception information -- all those are copied
because they are mentioned in the method declaration. While this might give an
impression that it's the result of `{@inheritDoc}`, it is important to
understand that it isn't. For example, since runtime exceptions and errors
aren't required to be mentioned in the `throws` clause of a method declaration,
no `@throws` tags corresponding to such throwables will be copied
automatically; the doc comment has to explicitly inherit those. In our case,
either would do:
/**
* @throws NoSuchElementException {@inheritDoc}
*/
public E getFirst() {
/**
* {@inheritDoc}
* @throws NoSuchElementException {@inheritDoc}
*/
public E getFirst() {
Speaking of which: @stuart-marks, do you think you could add those everywhere?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1171145153
Re: RFR: 8306075: Micro-optimize Enum.hashCode [v6]
On Wed, 19 Apr 2023 13:50:49 GMT, Aleksey Shipilev wrote: > @AlanBateman, @apangin, @ExE-Boss, @liach, @pavelrappo -- you had comments on > this PR, could you please see if those are still relevant or not addressed? > If you are good with the current version, can you please formally approve the > PR? Thanks! Don't mind me: I had minor, mostly unrelated comments. - PR Comment: https://git.openjdk.org/jdk/pull/13491#issuecomment-1514898960
Re: RFR: 8266571: Sequenced Collections [v7]
On Thu, 20 Apr 2023 04:13:57 GMT, Stuart Marks wrote: > I didn't know about the need to specify `@throws` in order to generate the > throws-clauses in the docs. You need to explicitly inherit `@throws` only for exceptions that aren't mentioned in the `throws` clause. > As an aside, I note that some methods such as ArrayList.addFirst/addLast had > this javadoc: > > ``` > /** > * @inheritDoc > */ > ``` > > It didn't appear at all in the javadoc output, I guess because of > `--override-methods=summary`. Correct: it shouldn't have appeared in "Method Details" because of that option. But if it didn't appear at all (i.e. not even in the "Method Summary / Methods declared in ..." table at the top of the page), it's a bug. > Is there a way to inherit the doc from a superclass but force a particular > method to have its details included? It's kind of moot because these are also > missing a `@since` tag, and adding that did cause the method detail to be > included. Other than to add to the inherited documentation? Not that I know of. Yes, adding a `@since` tag will cause the method detail to appear. - PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1172297190
Re: RFR: 8266571: Sequenced Collections [v7]
On Thu, 20 Apr 2023 16:19:05 GMT, Stuart Marks wrote: >>> I didn't know about the need to specify `@throws` in order to generate the >>> throws-clauses in the docs. >> >> You need to explicitly inherit `@throws` only for exceptions that aren't >> mentioned in the `throws` clause. >> >>> As an aside, I note that some methods such as ArrayList.addFirst/addLast >>> had this javadoc: >>> >>> ``` >>> /** >>> * @inheritDoc >>> */ >>> ``` >>> >>> It didn't appear at all in the javadoc output, I guess because of >>> `--override-methods=summary`. >> >> Correct: it shouldn't have appeared in "Method Details" because of that >> option. But if it didn't appear at all (i.e. not even in the "Method Summary >> / Methods declared in ..." table at the top of the page), it's a bug. >> >>> Is there a way to inherit the doc from a superclass but force a particular >>> method to have its details included? It's kind of moot because these are >>> also missing a `@since` tag, and adding that did cause the method detail to >>> be included. >> >> Other than to add to the inherited documentation? Not that I know of. Yes, >> adding a `@since` tag will cause the method detail to appear. > > Oh, oops, I was mistaken to say "It didn't appear at all in the javadoc > output." It did appear in the Method Summary, but not in the Method Details. > The behavior is thus as expected. However, it still seems like there's > something "off" about this situation. As a javadoc author it's hard to > predict what will happen. Maybe more control is necessary. In addition, from > the editorial standpoint of the specification, we also need to figure out > when something should appear in the Summary and when it should appear in the > Detail. Actually we should figure that out first, use it to drive the > appropriate mechanisms in the javadoc tool, and then adjust the doc comments > themselves as necessary. I note that other methods might have similar issues. For example, `NullPointerException` in `addFirst` and `addLast`. - PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1176243136
RFR: 8308735: Typos in parameter names
Please review this simple fix. - Commit messages: - Fix typos Changes: https://git.openjdk.org/jdk/pull/14136/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14136&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8308735 Stats: 9 lines in 3 files changed: 0 ins; 0 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/14136.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14136/head:pull/14136 PR: https://git.openjdk.org/jdk/pull/14136
Integrated: 8308735: Typos in parameter names
On Wed, 24 May 2023 23:32:18 GMT, Pavel Rappo wrote: > Please review this simple fix. This pull request has now been integrated. Changeset: 38367d3c Author: Pavel Rappo URL: https://git.openjdk.org/jdk/commit/38367d3c3ad9292b7c581917c89e9f07fac3dd31 Stats: 9 lines in 3 files changed: 0 ins; 0 del; 9 mod 8308735: Typos in parameter names Reviewed-by: naoto, iris, bpb - PR: https://git.openjdk.org/jdk/pull/14136
RFR: 8285368: Overhaul doc-comment inheritance
Please review this long-awaited change to documentation inheritance.
This change improves "methods comment algorithm" and introduces directed
documentation inheritance. While "methods comment algorithm" -- automatic
search for inheritable documentation -- has been improved, it still cannot read
an author's mind so as to always find the documentation they intended. From now
on, an author can state their intention, by providing an FQN of the superclass
or superinterface from which to inherit documentation:
{@inheritDoc S}
Which is exactly what I did to counterbalance some of the JDK API Documentation
changes caused by the change to "methods comment algorithm".
-
Commit messages:
- Disable problematic check
- Counterbalance changes to method comments algorithm
- Improve diagnostics
- Update method comments algorithm
- Implement directed documentation inheritance
Changes: https://git.openjdk.org/jdk/pull/14357/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14357&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8285368
Stats: 1827 lines in 32 files changed: 1693 ins; 29 del; 105 mod
Patch: https://git.openjdk.org/jdk/pull/14357.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14357/head:pull/14357
PR: https://git.openjdk.org/jdk/pull/14357
Re: RFR: 8285368: Overhaul doc-comment inheritance
On Wed, 7 Jun 2023 15:27:28 GMT, Jonathan Gibbons wrote:
>> Please review this long-awaited change to documentation inheritance.
>>
>> This change improves "methods comment algorithm" and introduces directed
>> documentation inheritance. While "methods comment algorithm" -- automatic
>> search for inheritable documentation -- has been improved, it still cannot
>> read an author's mind so as to always find the documentation they intended.
>> From now on, an author can state their intention, by providing an FQN of the
>> superclass or superinterface from which to inherit documentation:
>>
>> {@inheritDoc S}
>>
>> Which is exactly what I did to counterbalance some of the JDK API
>> Documentation changes caused by the change to "methods comment algorithm".
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
> line 1325:
>
>> 1323: //
>> 1324: // m.at(pos).newInheritDocTree(ref)
>> 1325: //
>
> Although the scary warning is not wrong, I think it would have been enough to
> say something more concise, like "_use the original legacy method if no ref
> is given_"
I'll tone it down; thanks
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r1221821265
Re: RFR: 8285368: Overhaul doc-comment inheritance
On Wed, 7 Jun 2023 14:19:16 GMT, Pavel Rappo wrote:
> Please review this long-awaited change to documentation inheritance.
>
> This change improves "methods comment algorithm" and introduces directed
> documentation inheritance. While "methods comment algorithm" -- automatic
> search for inheritable documentation -- has been improved, it still cannot
> read an author's mind so as to always find the documentation they intended.
> From now on, an author can state their intention, by providing an FQN of the
> superclass or superinterface from which to inherit documentation:
>
> {@inheritDoc S}
>
> Which is exactly what I did to counterbalance some of the JDK API
> Documentation changes caused by the change to "methods comment algorithm".
For details on changes to "methods comment algorithm", see the associated CSR:
https://bugs.openjdk.org/browse/JDK-8287152
-
PR Comment: https://git.openjdk.org/jdk/pull/14357#issuecomment-1581099793
Re: RFR: 8285368: Overhaul doc-comment inheritance [v2]
> Please review this long-awaited change to documentation inheritance.
>
> This change improves "methods comment algorithm" and introduces directed
> documentation inheritance. While "methods comment algorithm" -- automatic
> search for inheritable documentation -- has been improved, it still cannot
> read an author's mind so as to always find the documentation they intended.
> From now on, an author can state their intention, by providing an FQN of the
> superclass or superinterface from which to inherit documentation:
>
> {@inheritDoc S}
>
> Which is exactly what I did to counterbalance some of the JDK API
> Documentation changes caused by the change to "methods comment algorithm".
Pavel Rappo has updated the pull request incrementally with one additional
commit since the last revision:
feedback: make warning less scary
-
Changes:
- all: https://git.openjdk.org/jdk/pull/14357/files
- new: https://git.openjdk.org/jdk/pull/14357/files/a56253b3..c0fc69c2
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=14357&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=14357&range=00-01
Stats: 14 lines in 1 file changed: 0 ins; 12 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/14357.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14357/head:pull/14357
PR: https://git.openjdk.org/jdk/pull/14357
Re: RFR: 8285368: Overhaul doc-comment inheritance [v2]
On Wed, 7 Jun 2023 16:02:40 GMT, Pavel Rappo wrote:
>> Please review this long-awaited change to documentation inheritance.
>>
>> This change improves "methods comment algorithm" and introduces directed
>> documentation inheritance. While "methods comment algorithm" -- automatic
>> search for inheritable documentation -- has been improved, it still cannot
>> read an author's mind so as to always find the documentation they intended.
>> From now on, an author can state their intention, by providing an FQN of the
>> superclass or superinterface from which to inherit documentation:
>>
>> {@inheritDoc S}
>>
>> Which is exactly what I did to counterbalance some of the JDK API
>> Documentation changes caused by the change to "methods comment algorithm".
>
> Pavel Rappo has updated the pull request incrementally with one additional
> commit since the last revision:
>
> feedback: make warning less scary
Full disclosure. Here are all the files that are different before (1) and after
(2) the change:
- https://cr.openjdk.org/~prappo/8285368/1/api
- https://cr.openjdk.org/~prappo/8285368/2/api
As you can see, the vast majority of differences are in "See Also" sections,
which come from `@see` tags. Sadly, `@see` tags are inheritable "by omission".
That said, `@see` is also underspecified, somewhat broken, and I believe is not
as important as those parts of a doc comment that allow `{@inheritDoc}`.
I suggest that we integrate this PR and take care of `@see` in 22. But if the
corrections are really required, I can try to introduce explicit `@see` tags,
so there are no changes whatsoever.
-
PR Comment: https://git.openjdk.org/jdk/pull/14357#issuecomment-1581226538
Re: RFR: 8285368: Overhaul doc-comment inheritance [v2]
On Wed, 7 Jun 2023 20:29:07 GMT, Jonathan Gibbons wrote: >> Pavel Rappo has updated the pull request incrementally with one additional >> commit since the last revision: >> >> feedback: make warning less scary > > test/langtools/jdk/javadoc/doclet/testDirectedInheritance/TestDirectedInheritance.java > line 673: > >> 671: * >> 672: * For now a warning is issued if a doc comment inherits from >> 673: * an indirect supertype. > > Not sure I agree with this. Can we discuss? Sure we can; this relates to an earlier comment of yours on Utils.isDirectSupertype here: https://github.com/openjdk/jdk/pull/14357#discussion_r1222053011 - PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r1222141331
Re: RFR: 8285368: Overhaul doc-comment inheritance [v2]
On Wed, 7 Jun 2023 19:13:55 GMT, Jonathan Gibbons wrote: >> Pavel Rappo has updated the pull request incrementally with one additional >> commit since the last revision: >> >> feedback: make warning less scary > > src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java > line 671: > >> 669: //.anyMatch(t -> Objects.equals(typeUtils.asElement(t), >> typeUtils.asElement(t2))); >> 670: >> 671: return true; /* disabled for causing issues in JDK API >> Documentation build */ > > Please describe the issues and/or provide a JBS issue, so that we can decide > when to revert the code to the commented-out form. I mentioned it in the test, here: TestDirectedInheritance.testNotDirectSupertype. I reckon referring to code from a test is okay, but not the other way around. - PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r1222148500
Re: RFR: 8285368: Overhaul doc-comment inheritance [v2]
On Wed, 7 Jun 2023 19:16:51 GMT, Jonathan Gibbons wrote:
>> Pavel Rappo has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> feedback: make warning less scary
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java
> line 2847:
>
>> 2845: break;
>> 2846: }
>> 2847: if (isPlainInterface(peek) && !isPublic(peek) &&
>> !isLinkable(peek)) {
>
> What is the significance of the `isPublic` check?
> I'm developing a knee-jerk reaction to such checks, in the face of
> access-related command-line options.
That directly mimics the current behaviour at
addSuperInterfaces(Utils.java:802), which in turn is used at
ImplementedMethods(VisibleMemberTable.java:1064) to collect overridden methods
in superinterfaces. If you look around, it's quite an idiom and predates this
PR.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r1222173441
Re: RFR: 8285368: Overhaul doc-comment inheritance [v3]
> Please review this long-awaited change to documentation inheritance.
>
> This change improves "methods comment algorithm" and introduces directed
> documentation inheritance. While "methods comment algorithm" -- automatic
> search for inheritable documentation -- has been improved, it still cannot
> read an author's mind so as to always find the documentation they intended.
> From now on, an author can state their intention, by providing an FQN of the
> superclass or superinterface from which to inherit documentation:
>
> {@inheritDoc S}
>
> Which is exactly what I did to counterbalance some of the JDK API
> Documentation changes caused by the change to "methods comment algorithm".
Pavel Rappo has updated the pull request with a new target base due to a merge
or a rebase. The incremental webrev excludes the unrelated changes brought in
by the merge/rebase. The pull request contains seven additional commits since
the last revision:
- Merge branch 'master' into 8285368
- feedback: make warning less scary
- Disable problematic check
This check is well-intended but problematic for JDK API Documentation
build, which errors on warnings.
- Counterbalance changes to method comments algorithm
The changes are in the JDK API Documentation.
- Improve diagnostics
- Update method comments algorithm
- Implement directed documentation inheritance
-
Changes:
- all: https://git.openjdk.org/jdk/pull/14357/files
- new: https://git.openjdk.org/jdk/pull/14357/files/c0fc69c2..011d7982
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=14357&range=02
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=14357&range=01-02
Stats: 17117 lines in 199 files changed: 16677 ins; 145 del; 295 mod
Patch: https://git.openjdk.org/jdk/pull/14357.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14357/head:pull/14357
PR: https://git.openjdk.org/jdk/pull/14357
Re: RFR: 8285368: Overhaul doc-comment inheritance [v4]
> Please review this long-awaited change to documentation inheritance.
>
> This change improves "methods comment algorithm" and introduces directed
> documentation inheritance. While "methods comment algorithm" -- automatic
> search for inheritable documentation -- has been improved, it still cannot
> read an author's mind so as to always find the documentation they intended.
> From now on, an author can state their intention, by providing an FQN of the
> superclass or superinterface from which to inherit documentation:
>
> {@inheritDoc S}
>
> Which is exactly what I did to counterbalance some of the JDK API
> Documentation changes caused by the change to "methods comment algorithm".
Pavel Rappo has updated the pull request incrementally with one additional
commit since the last revision:
feedback: use utils directly
-
Changes:
- all: https://git.openjdk.org/jdk/pull/14357/files
- new: https://git.openjdk.org/jdk/pull/14357/files/011d7982..d579a83c
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=14357&range=03
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=14357&range=02-03
Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/14357.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14357/head:pull/14357
PR: https://git.openjdk.org/jdk/pull/14357
Re: RFR: 8285368: Overhaul doc-comment inheritance [v2]
On Wed, 7 Jun 2023 18:55:10 GMT, Jonathan Gibbons wrote:
>> Pavel Rappo has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> feedback: make warning less scary
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/InheritDocTaglet.java
> line 110:
>
>> 108: for (Element e : methods) {
>> 109: ExecutableElement m = (ExecutableElement) e;
>> 110: if (configuration.utils.elementUtils.overrides(method,
>> m, (TypeElement) method.getEnclosingElement())) {
>
> Note that `configuration.utils` is available as `utils`
Done in d579a83cc4a.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r1222652470
Re: RFR: 8285368: Overhaul doc-comment inheritance [v2]
On Wed, 7 Jun 2023 20:23:09 GMT, Jonathan Gibbons wrote:
>> Pavel Rappo has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> feedback: make warning less scary
>
> test/langtools/jdk/javadoc/doclet/testDirectedInheritance/TestDirectedInheritance.java
> line 75:
>
>> 73: // code is OK, it likely isn't (after all, there's an unknown
>> tag).
>> 74: setAutomaticCheckNoStacktrace(true);
>> 75: { // DocLint is on
>
> "on" -> "explicit"
Done in 93adde63ed0.
> test/langtools/jdk/javadoc/doclet/testDirectedInheritance/TestDirectedInheritance.java
> line 85:
>
>> 83: }
>> 84: }
>> 85: // DocLint is off
>
> "off" -> "default"
Done in 93adde63ed0.
> test/langtools/jdk/javadoc/doclet/testDirectedInheritance/TestDirectedInheritance.java
> line 289:
>
>> 287:
>> 288: /*
>> 289: * C1.m inherits documentation from B1.m explicitly and undirect.
>
> possible typo: do you mean "indirect"
I'm neither a grammarian nor a native English speaker, but my hunch is that
_indirect_ would be wrong here. But again, I'm open to corrections.
I believe "directed inhertiance" came from an analogy with the graph theory,
where a graph can be directed and undirected. The former has pointy arrows, the
latter does not.
By that analogy, documentation inheritance can be directed `{@inheritDoc
}` (I choose), or undirected `{@inheritDoc}` (JavaDoc chooses / I'm
Feeling Lucky). In addition to that, documentation inheritance can be implicit
(i.e. by omission), which works the same way as undirected but without extra
keystrokes.
While those concepts are lacking better names, can I suggest we use the
following almost-made-up adverbs: _directedly_ and _undirectedly_? As a side
note, we badly need a JavaDoc vocabulary.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r1222703693
PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r1222703773
PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r1222700845
Re: RFR: 8285368: Overhaul doc-comment inheritance [v5]
> Please review this long-awaited change to documentation inheritance.
>
> This change improves "methods comment algorithm" and introduces directed
> documentation inheritance. While "methods comment algorithm" -- automatic
> search for inheritable documentation -- has been improved, it still cannot
> read an author's mind so as to always find the documentation they intended.
> From now on, an author can state their intention, by providing an FQN of the
> superclass or superinterface from which to inherit documentation:
>
> {@inheritDoc S}
>
> Which is exactly what I did to counterbalance some of the JDK API
> Documentation changes caused by the change to "methods comment algorithm".
Pavel Rappo has updated the pull request incrementally with one additional
commit since the last revision:
feedback: DocLint terminology
-
Changes:
- all: https://git.openjdk.org/jdk/pull/14357/files
- new: https://git.openjdk.org/jdk/pull/14357/files/d579a83c..93adde63
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=14357&range=04
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=14357&range=03-04
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/14357.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14357/head:pull/14357
PR: https://git.openjdk.org/jdk/pull/14357
Re: RFR: 8285368: Overhaul doc-comment inheritance [v6]
> Please review this long-awaited change to documentation inheritance.
>
> This change improves "methods comment algorithm" and introduces directed
> documentation inheritance. While "methods comment algorithm" -- automatic
> search for inheritable documentation -- has been improved, it still cannot
> read an author's mind so as to always find the documentation they intended.
> From now on, an author can state their intention, by providing an FQN of the
> superclass or superinterface from which to inherit documentation:
>
> {@inheritDoc S}
>
> Which is exactly what I did to counterbalance some of the JDK API
> Documentation changes caused by the change to "methods comment algorithm".
Pavel Rappo has updated the pull request incrementally with one additional
commit since the last revision:
suggestion: vocabulary
-
Changes:
- all: https://git.openjdk.org/jdk/pull/14357/files
- new: https://git.openjdk.org/jdk/pull/14357/files/93adde63..3da4f473
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=14357&range=05
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=14357&range=04-05
Stats: 11 lines in 1 file changed: 5 ins; 0 del; 6 mod
Patch: https://git.openjdk.org/jdk/pull/14357.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14357/head:pull/14357
PR: https://git.openjdk.org/jdk/pull/14357
Re: RFR: 8285368: Overhaul doc-comment inheritance [v2]
On Thu, 8 Jun 2023 09:18:04 GMT, Pavel Rappo wrote:
>> test/langtools/jdk/javadoc/doclet/testDirectedInheritance/TestDirectedInheritance.java
>> line 289:
>>
>>> 287:
>>> 288: /*
>>> 289: * C1.m inherits documentation from B1.m explicitly and undirect.
>>
>> possible typo: do you mean "indirect"
>
> I'm neither a grammarian nor a native English speaker, but my hunch is that
> _indirect_ would be wrong here. But again, I'm open to corrections.
>
> I believe "directed inhertiance" came from an analogy with the graph theory,
> where a graph can be directed and undirected. The former has pointy arrows,
> the latter does not.
>
> By that analogy, documentation inheritance can be directed `{@inheritDoc
> }` (I choose), or undirected `{@inheritDoc}` (JavaDoc chooses / I'm
> Feeling Lucky). In addition to that, documentation inheritance can be
> implicit (i.e. by omission), which works the same way as undirected but
> without extra keystrokes.
>
> While those concepts are lacking better names, can I suggest we use the
> following almost-made-up adverbs: _directedly_ and _undirectedly_? As a side
> note, we badly need a JavaDoc vocabulary.
Have a look at this commit: 3da4f473a7c
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r1222709203
Re: RFR: 8285368: Overhaul doc-comment inheritance [v2]
On Wed, 7 Jun 2023 21:39:33 GMT, Jonathan Gibbons wrote: >> The general criticism here is whether we should restrict in any way the set >> of super types from which one can inherit documentation, and/or what should >> the set of checks be? >> >> For example, if a method in C inherits a method in B that has no comment but >> which inherits a method with a comment in A, then it seems unduly >> restrictive to stop the method in C explicitly referencing the method in A, >> as compared to only allowing a reference to B, which implicitly gets its >> comment from A. >> >> That being said, there is merit in starting off with restrictions and >> loosening them in the face of experience, rather than trying to increase the >> stylistic restrictions later. > >> Sure we can; this relates to an earlier comment of yours on >> Utils.isDirectSupertype here: [#14357 >> (comment)](https://github.com/openjdk/jdk/pull/14357#discussion_r1222053011) > > Thanks for tying these together; I had not realized they were related. As agreed out of band, I removed that check and the test for now in dea776e285b. - PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r1222737390
Re: RFR: 8285368: Overhaul doc-comment inheritance [v7]
> Please review this long-awaited change to documentation inheritance.
>
> This change improves "methods comment algorithm" and introduces directed
> documentation inheritance. While "methods comment algorithm" -- automatic
> search for inheritable documentation -- has been improved, it still cannot
> read an author's mind so as to always find the documentation they intended.
> From now on, an author can state their intention, by providing an FQN of the
> superclass or superinterface from which to inherit documentation:
>
> {@inheritDoc S}
>
> Which is exactly what I did to counterbalance some of the JDK API
> Documentation changes caused by the change to "methods comment algorithm".
Pavel Rappo has updated the pull request incrementally with one additional
commit since the last revision:
feedback: remove unduly restrictive warning
-
Changes:
- all: https://git.openjdk.org/jdk/pull/14357/files
- new: https://git.openjdk.org/jdk/pull/14357/files/3da4f473..dea776e2
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=14357&range=06
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=14357&range=05-06
Stats: 126 lines in 5 files changed: 0 ins; 126 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/14357.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14357/head:pull/14357
PR: https://git.openjdk.org/jdk/pull/14357
Re: RFR: 8285368: Overhaul doc-comment inheritance [v7]
On Thu, 8 Jun 2023 10:33:17 GMT, Alan Bateman wrote:
>> Pavel Rappo has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> feedback: remove unduly restrictive warning
>
> src/java.base/share/classes/java/util/TreeMap.java line 1199:
>
>> 1197: * {@code add} or {@code addAll} operations.
>> 1198: *
>> 1199: * @return {@inheritDoc SortedMap}
>
> An alternative here might be add `{@return ...}` to the first sentence of
> the method description so that the return description exactly matches.
If we were to (re-)structure doc comments in the way you propose, I'd suggest
we do it in a separate, non-jdk.javadoc PR. I was simply trying to keep JDK
API documentation unchanged.
One thing with the `@return` documentation inherited from SortedMap is that
compared to the first sentence of TreeMap, it provides one extra bit of
information: a set view of the mappings contained in this map, **sorted in
ascending key order**. In order to keep that bit, the suggested inline
`{@return ...}` should include more than just the first sentence.
> src/java.base/share/classes/java/util/concurrent/LinkedBlockingDeque.java
> line 635:
>
>> 633: * @return {@inheritDoc BlockingDeque}
>> 634: */
>> 635: public boolean offer(E e) {
>
> Does this work for @param tags too? Just curious.
The directed `{@inheritDoc }` works for the main description, `@throws`,
`@param` and `@return` tags. If your question is about why that particular doc
comment does not use this:
@param e {@inheritDoc BlockingDeque}
then the answer is that it's not necessary for keeping the API documentation
unchanged.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r1222908801
PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r1222910706
Re: RFR: 8285368: Overhaul doc-comment inheritance [v7]
On Thu, 8 Jun 2023 09:55:27 GMT, Pavel Rappo wrote:
>> Please review this long-awaited change to documentation inheritance.
>>
>> This change improves "methods comment algorithm" and introduces directed
>> documentation inheritance. While "methods comment algorithm" -- automatic
>> search for inheritable documentation -- has been improved, it still cannot
>> read an author's mind so as to always find the documentation they intended.
>> From now on, an author can state their intention, by providing an FQN of the
>> superclass or superinterface from which to inherit documentation:
>>
>> {@inheritDoc S}
>>
>> Which is exactly what I did to counterbalance some of the JDK API
>> Documentation changes caused by the change to "methods comment algorithm".
>
> Pavel Rappo has updated the pull request incrementally with one additional
> commit since the last revision:
>
> feedback: remove unduly restrictive warning
@Martin-Buchholz, could you please have a look at changes to collections and,
in particular, those in java.util.concurrent.*?
-
PR Comment: https://git.openjdk.org/jdk/pull/14357#issuecomment-1582426421
Re: RFR: 8285368: Overhaul doc-comment inheritance [v7]
On Thu, 8 Jun 2023 09:55:27 GMT, Pavel Rappo wrote:
>> Please review this long-awaited change to documentation inheritance.
>>
>> This change improves "methods comment algorithm" and introduces directed
>> documentation inheritance. While "methods comment algorithm" -- automatic
>> search for inheritable documentation -- has been improved, it still cannot
>> read an author's mind so as to always find the documentation they intended.
>> From now on, an author can state their intention, by providing an FQN of the
>> superclass or superinterface from which to inherit documentation:
>>
>> {@inheritDoc S}
>>
>> Which is exactly what I did to counterbalance some of the JDK API
>> Documentation changes caused by the change to "methods comment algorithm".
>
> Pavel Rappo has updated the pull request incrementally with one additional
> commit since the last revision:
>
> feedback: remove unduly restrictive warning
Note that the change to "method comments algorithm" uncovers and fixes some
bugs to Object- or Object-like- methods.
Using the provided [before][] and [after][] outputs, compare documentation for:
* toString() in java.time.chrono.{Hijrah,Japanese,Minguo,ThaiBuddhist}Date
* hashCode() in Hashtable, ConcurrentHashMap.KeySetView,
java.util.jar.Attributes, com.sun.net.httpserver.Headers,
com.sun.management.GcInfo
* equals() in ConcurrentHashMap.KeySetView, com.sun.net.httpserver.Headers,
com.sun.management.GcInfo
* clone() in javax.management.ImmutableDescriptor,
javax.management.modelmbean.DescriptorSupport,
javax.naming.directory.BasicAttribute, javax.naming.directory.BasicAttributes
Perhaps some of those could be further refined using directed documentation
inheritance. For example, ConcurrentHashMap.KeySetView is first and foremost a
set and only then a collection. That suggests that the documentation for
Object-like methods could be inherited from (some) set rather than a "generic"
collection.
Also, the javax.management.ImmutableDescriptor and
javax.management.modelmbean.DescriptorSupport clone methods require some
attention from area experts. It's likely that those methods' doc comments need
to fully inherit documentation from Descriptor.clone(), not just its `@return`
tag.
[before]: https://cr.openjdk.org/~prappo/8285368/1/api
[after]: https://cr.openjdk.org/~prappo/8285368/2/api
-
PR Comment: https://git.openjdk.org/jdk/pull/14357#issuecomment-1582477351
Re: RFR: 8285368: Overhaul doc-comment inheritance [v8]
> Please review this long-awaited change to documentation inheritance.
>
> This change improves "methods comment algorithm" and introduces directed
> documentation inheritance. While "methods comment algorithm" -- automatic
> search for inheritable documentation -- has been improved, it still cannot
> read an author's mind so as to always find the documentation they intended.
> From now on, an author can state their intention, by providing an FQN of the
> superclass or superinterface from which to inherit documentation:
>
> {@inheritDoc S}
>
> Which is exactly what I did to counterbalance some of the JDK API
> Documentation changes caused by the change to "methods comment algorithm".
Pavel Rappo has updated the pull request incrementally with one additional
commit since the last revision:
feedback: use JavadocTester.out
Also, trivially fixes grammar (word order) in a comment.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/14357/files
- new: https://git.openjdk.org/jdk/pull/14357/files/dea776e2..cccffa66
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=14357&range=07
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=14357&range=06-07
Stats: 6 lines in 1 file changed: 0 ins; 0 del; 6 mod
Patch: https://git.openjdk.org/jdk/pull/14357.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14357/head:pull/14357
PR: https://git.openjdk.org/jdk/pull/14357
Re: RFR: 8285368: Overhaul doc-comment inheritance [v2]
On Wed, 7 Jun 2023 20:33:00 GMT, Jonathan Gibbons wrote:
>> Pavel Rappo has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> feedback: make warning less scary
>
> test/langtools/jdk/javadoc/doclet/testMethodCommentAlgorithm/TestMethodCommentsAlgorithm.java
> line 273:
>
>> 271: }
>> 272: }
>> 273: System.err.println("Test suite root: " + p);
>
> You might want to use `JavadocTester.out` instead of `System.err`
Fixed in cccffa660a8.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r1223144388
Re: RFR: 8285368: Overhaul doc-comment inheritance [v7]
On Thu, 8 Jun 2023 15:16:09 GMT, Alexey Ivanov wrote:
>> The directed `{@inheritDoc }` works for the main description,
>> `@throws`, `@param` and `@return` tags. If your question is about why that
>> particular doc comment does not use this:
>>
>> @param e {@inheritDoc BlockingDeque}
>>
>> then the answer is that it's not necessary for keeping the API documentation
>> unchanged.
>
> I admit I can't parse this sentence:
>
>> then the answer is that it's not necessary for keeping the API documentation
>> unchanged.
>
> Do you mean that you didn't add `@param e {@inheritDoc BlockingDeque}` here
> to keep the documentation unchanged?
>
> You added `@inheritDoc` for `@param` tags in
> [FileCacheImageOutputStream.java](https://github.com/openjdk/jdk/pull/14357/files#diff-bc1e47b4c5c405826e7a062cabea04349ef72d5823b67e8616a680dcfcb5d4be)
> and others but not here.
>
> So, do we need to add `@param` tags if the entire javadoc is inherited with
> `@inheritDoc`?
Copying an individual doc element using `{@inheritDoc}` never adds meta
information on the generated HTML page to indicate where that doc element was
copied from. It so happens in this particular case that the doc element's
contents are the same in both possible supertypes (BlockingDeque and Queue) and
look like this:
@param e the element to add
If the end result _on the HTML page_ will look identical no mater if I add
`@param e {@inheritDoc BlockingDeque}` or not, I will not add it in _this_ PR.
This PR's goal is not restructuring the doc comment; in fact, such
restructuring would be an anti-goal. It might be a goal for subsequent,
area-specific PRs.
Above I said that "`{@inheritDoc}` never adds meta information on the generated
HTML page". You might have immediately thought about cases where you saw the
"Description copied from interface/class" headings. However, those do not come
from `{@inheritDoc}`; those come from empty comments (which inherit their
content by omission, if you wish). So those headings DO indicate the source of
the complete doc comment.
Now about FileCacheImageOutputStream.java. If I hand't added directed
inheritance there, the updated algorithm would've picked a different comment,
which wouldn't have been identical to what was picked before:
- javax.imageio.stream.ImageInputStream#flushBefore
```
* @param pos a {@code long} containing the length of the
* stream prefix that may be flushed.
```
- javax.imageio.stream.ImageOutputStream#flushBefore:
```
* @param pos a {@code long} containing the length of the
* stream prefix that may be flushed to the destination.
```
Hence, the correction. Does it clarify the matter?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r1223236596
Re: RFR: 8285368: Overhaul doc-comment inheritance [v7]
On Thu, 8 Jun 2023 15:47:26 GMT, Pavel Rappo wrote:
>> I admit I can't parse this sentence:
>>
>>> then the answer is that it's not necessary for keeping the API
>>> documentation unchanged.
>>
>> Do you mean that you didn't add `@param e {@inheritDoc BlockingDeque}` here
>> to keep the documentation unchanged?
>>
>> You added `@inheritDoc` for `@param` tags in
>> [FileCacheImageOutputStream.java](https://github.com/openjdk/jdk/pull/14357/files#diff-bc1e47b4c5c405826e7a062cabea04349ef72d5823b67e8616a680dcfcb5d4be)
>> and others but not here.
>>
>> So, do we need to add `@param` tags if the entire javadoc is inherited with
>> `@inheritDoc`?
>
> Copying an individual doc element using `{@inheritDoc}` never adds meta
> information on the generated HTML page to indicate where that doc element was
> copied from. It so happens in this particular case that the doc element's
> contents are the same in both possible supertypes (BlockingDeque and Queue)
> and look like this:
>
> @param e the element to add
>
> If the end result _on the HTML page_ will look identical no mater if I add
> `@param e {@inheritDoc BlockingDeque}` or not, I will not add it in _this_
> PR. This PR's goal is not restructuring the doc comment; in fact, such
> restructuring would be an anti-goal. It might be a goal for subsequent,
> area-specific PRs.
>
> Above I said that "`{@inheritDoc}` never adds meta information on the
> generated HTML page". You might have immediately thought about cases where
> you saw the "Description copied from interface/class" headings. However,
> those do not come from `{@inheritDoc}`; those come from empty comments (which
> inherit their content by omission, if you wish). So those headings DO
> indicate the source of the complete doc comment.
>
> Now about FileCacheImageOutputStream.java. If I hand't added directed
> inheritance there, the updated algorithm would've picked a different comment,
> which wouldn't have been identical to what was picked before:
>
> - javax.imageio.stream.ImageInputStream#flushBefore
> ```
> * @param pos a {@code long} containing the length of the
> * stream prefix that may be flushed.
> ```
>
> - javax.imageio.stream.ImageOutputStream#flushBefore:
> ```
> * @param pos a {@code long} containing the length of the
> * stream prefix that may be flushed to the destination.
> ```
> Hence, the correction. Does it clarify the matter?
Also, for better or worse, a lone `{@inheritDoc}` never inherits "everything"
from a supertype:
/** {@inheritDoc} */
It merely inherits the main description of a method. Other elements such as
parameter, exception or return information are inherited individually.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r1223241132
Re: RFR: 8285368: Overhaul doc-comment inheritance [v9]
> Please review this long-awaited change to documentation inheritance.
>
> This change improves "methods comment algorithm" and introduces directed
> documentation inheritance. While "methods comment algorithm" -- automatic
> search for inheritable documentation -- has been improved, it still cannot
> read an author's mind so as to always find the documentation they intended.
> From now on, an author can state their intention, by providing an FQN of the
> superclass or superinterface from which to inherit documentation:
>
> {@inheritDoc S}
>
> Which is exactly what I did to counterbalance some of the JDK API
> Documentation changes caused by the change to "methods comment algorithm".
Pavel Rappo has updated the pull request with a new target base due to a merge
or a rebase. The incremental webrev excludes the unrelated changes brought in
by the merge/rebase. The pull request contains 14 additional commits since the
last revision:
- fix failing SourceDocTreeScannerTest
- Merge branch 'master' into 8285368
- feedback: use JavadocTester.out
Also, trivially fixes grammar (word order) in a comment.
- feedback: remove unduly restrictive warning
- suggestion: vocabulary
- feedback: DocLint terminology
- feedback: use utils directly
- Merge branch 'master' into 8285368
- feedback: make warning less scary
- Disable problematic check
This check is well-intended but problematic for JDK API Documentation
build, which errors on warnings.
- ... and 4 more: https://git.openjdk.org/jdk/compare/e7a50303...563a8761
-
Changes:
- all: https://git.openjdk.org/jdk/pull/14357/files
- new: https://git.openjdk.org/jdk/pull/14357/files/cccffa66..563a8761
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=14357&range=08
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=14357&range=07-08
Stats: 11904 lines in 355 files changed: 9502 ins; 1007 del; 1395 mod
Patch: https://git.openjdk.org/jdk/pull/14357.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14357/head:pull/14357
PR: https://git.openjdk.org/jdk/pull/14357
Re: RFR: 8285368: Overhaul doc-comment inheritance [v8]
On Thu, 8 Jun 2023 14:47:13 GMT, Pavel Rappo wrote:
>> Please review this long-awaited change to documentation inheritance.
>>
>> This change improves "methods comment algorithm" and introduces directed
>> documentation inheritance. While "methods comment algorithm" -- automatic
>> search for inheritable documentation -- has been improved, it still cannot
>> read an author's mind so as to always find the documentation they intended.
>> From now on, an author can state their intention, by providing an FQN of the
>> superclass or superinterface from which to inherit documentation:
>>
>> {@inheritDoc S}
>>
>> Which is exactly what I did to counterbalance some of the JDK API
>> Documentation changes caused by the change to "methods comment algorithm".
>
> Pavel Rappo has updated the pull request incrementally with one additional
> commit since the last revision:
>
> feedback: use JavadocTester.out
>
> Also, trivially fixes grammar (word order) in a comment.
core-libs, client: any reviewers?
-
PR Comment: https://git.openjdk.org/jdk/pull/14357#issuecomment-1588987444
Re: RFR: 8285368: Overhaul doc-comment inheritance [v9]
On Wed, 14 Jun 2023 01:19:57 GMT, Martin Buchholz wrote: > We don't currently have a way to diff rendered javadoc, like my ancient > BlenderRev hack? I've generated something you might find helpful: https://cr.openjdk.org/~prappo/8285368/specdiff/overview-summary.html - PR Comment: https://git.openjdk.org/jdk/pull/14357#issuecomment-1590779943
Re: RFR: 8285368: Overhaul doc-comment inheritance [v9]
On Wed, 14 Jun 2023 01:15:51 GMT, Martin Buchholz wrote:
> * we never want to inherit AbstractFoo @implNotes
`@implNote`, `@apiNote` and `@implSpec` can only be inherited via explicit
`{@inheritDoc}`:
@implSpec {@inheritDoc}
> * ConcurrentMap does _not_ have the same spec as ConcurrentHashMap, e.g. the
> latter does not permit null values. Therefore one can argue that javadoc
> should _not_ be inherited here. Right now the main method spec from
> ConcurrentMap is perfectly suitable for ConcurrentHashMap, but one can
> imagine a future change to ConcurrentMap::putIfAbsent's spec that changes
> that, perhaps due to the null value handling difference. We have a
> distasteful choice - brittle inheritance or copy-pasta. In practice not so
> bad here, since these classes are maintained together.
That state of affairs predates this PR and is merely highlighted by it. Sadly,
I'm not sure how JavaDoc can help here. Annotations, contracts, inspections,
and doc tests come to mind; but none of these are supported by JavaDoc at the
moment.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r1229354100
Re: RFR: 8285368: Overhaul doc-comment inheritance [v10]
> Please review this long-awaited change to documentation inheritance.
>
> This change improves "methods comment algorithm" and introduces directed
> documentation inheritance. While "methods comment algorithm" -- automatic
> search for inheritable documentation -- has been improved, it still cannot
> read an author's mind so as to always find the documentation they intended.
> From now on, an author can state their intention, by providing an FQN of the
> superclass or superinterface from which to inherit documentation:
>
> {@inheritDoc S}
>
> Which is exactly what I did to counterbalance some of the JDK API
> Documentation changes caused by the change to "methods comment algorithm".
Pavel Rappo has updated the pull request with a new target base due to a merge
or a rebase. The incremental webrev excludes the unrelated changes brought in
by the merge/rebase. The pull request contains 17 additional commits since the
last revision:
- Merge branch 'master' into 8285368
- Update @since tags
- Add a test
- fix failing SourceDocTreeScannerTest
- Merge branch 'master' into 8285368
- feedback: use JavadocTester.out
Also, trivially fixes grammar (word order) in a comment.
- feedback: remove unduly restrictive warning
- suggestion: vocabulary
- feedback: DocLint terminology
- feedback: use utils directly
- ... and 7 more: https://git.openjdk.org/jdk/compare/5a217d2b...5b6af244
-
Changes:
- all: https://git.openjdk.org/jdk/pull/14357/files
- new: https://git.openjdk.org/jdk/pull/14357/files/563a8761..5b6af244
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=14357&range=09
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=14357&range=08-09
Stats: 8391 lines in 228 files changed: 1225 ins; 6803 del; 363 mod
Patch: https://git.openjdk.org/jdk/pull/14357.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14357/head:pull/14357
PR: https://git.openjdk.org/jdk/pull/14357
Integrated: 8285368: Overhaul doc-comment inheritance
On Wed, 7 Jun 2023 14:19:16 GMT, Pavel Rappo wrote:
> Please review this long-awaited change to documentation inheritance.
>
> This change improves "methods comment algorithm" and introduces directed
> documentation inheritance. While "methods comment algorithm" -- automatic
> search for inheritable documentation -- has been improved, it still cannot
> read an author's mind so as to always find the documentation they intended.
> From now on, an author can state their intention, by providing an FQN of the
> superclass or superinterface from which to inherit documentation:
>
> {@inheritDoc S}
>
> Which is exactly what I did to counterbalance some of the JDK API
> Documentation changes caused by the change to "methods comment algorithm".
This pull request has now been integrated.
Changeset: 3e0bbd29
Author:Pavel Rappo
URL:
https://git.openjdk.org/jdk/commit/3e0bbd290c534b0f9729c54cd45308d505907797
Stats: 1768 lines in 32 files changed: 1634 ins; 32 del; 102 mod
8285368: Overhaul doc-comment inheritance
6376959: Algorithm for Inheriting Method Comments seems to go not as documented
6934301: Support directed inheriting of class comments with @inheritDoc
Reviewed-by: jjg, rriggs, aivanov, smarks, martin
-
PR: https://git.openjdk.org/jdk/pull/14357
Re: RFR: 8310033: Clarify return value of Java Time compareTo methods [v2]
On Fri, 16 Jun 2023 22:12:43 GMT, Roger Riggs wrote:
>> In java.time packages, clarify timeline order javadoc to mention "before"
>> and "after" in the value of the `compareTo` method return values.
>> Add javadoc @see tags to isBefore and isAfter methods
>>
>> Replace use of "negative" and positive with "less than zero" and "greater
>> than zero" in javadoc @return
>> The term "positive" is ambiguous, zero is considered positive and indicates
>> equality.
>
> Roger Riggs has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains five additional
> commits since the last revision:
>
> - Use {@code xxx} to highlight the comparison against the arg.
>Update copyrights.
> - Merge branch 'master' into 8310033-time-compareto
> - Clarify for Duration, AbstractChronology, and Chronology
> - Correct javadoc of compareInstant
> - 8310033: Improve Instant.compareTo javadoc to mention before and after
>Refine timeline order to mention before and after
>Add javadoc @see tags to isBefore and isAfter methods
While the new wording is clearly an improvement, this reads weird:
> @return the comparator value is less...
For readability, I'd use _which_ or _that_, depending on your preferred
flavour/flavor of English.
@return the comparator value that is less...
-
PR Comment: https://git.openjdk.org/jdk/pull/14479#issuecomment-1595417780
Re: RFR: JDK-8310453: Update javadoc of java.lang.Object
On Tue, 20 Jun 2023 17:13:26 GMT, Joe Darcy wrote:
> I had occasion to read over the javadoc sources in java.lang.Object recently
> and noticed a few items that could be updated.
>
> There are some new or expanded API notes referring to methods in
> java.util.Objects. I added these references as apiNote items rather than,
> say, `@see` tags since `@see` tag changes would propagate into classes that
> overrode the methods in questions.
>
> Changing toString to use an inline `@return` tag has the consequence of
> omitting a trailing period, "." in the "Returns" section of its javadoc. This
> also omits a trailing period in subclasses that use the `@return` statement
> of Object.toString in their own toString method. Likewise for hashCode.
src/java.base/share/classes/java/lang/Object.java line 260:
> 258: * }
> 259: * The {@link java.util.Objects#toIdentityString(Object)
> 260: * Objects.toIdentityString} method returns the string for an
That last sentence doesn't feel like it needs to be part of `@implSpec`,
although there's definitely a connection.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14567#discussion_r1236859323
Re: RFR: JDK-8310453: Update javadoc of java.lang.Object
On Tue, 20 Jun 2023 17:13:26 GMT, Joe Darcy wrote:
> I had occasion to read over the javadoc sources in java.lang.Object recently
> and noticed a few items that could be updated.
>
> There are some new or expanded API notes referring to methods in
> java.util.Objects. I added these references as apiNote items rather than,
> say, `@see` tags since `@see` tag changes would propagate into classes that
> overrode the methods in questions.
>
> Changing toString to use an inline `@return` tag has the consequence of
> omitting a trailing period, "." in the "Returns" section of its javadoc. This
> also omits a trailing period in subclasses that use the `@return` statement
> of Object.toString in their own toString method. Likewise for hashCode.
src/java.base/share/classes/java/lang/Object.java line 260:
> 258: * }
> 259: * The {@link java.util.Objects#toIdentityString(Object)
> 260: * Objects.toIdentityString} method returns the string for an
Separately, is it just me or this sentence could be rephrased to not read like
`toIdentityString(Object)` accepts an object equal to the string (that would
...)?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14567#discussion_r1236870099
Re: RFR: JDK-8310453: Update javadoc of java.lang.Object
On Wed, 21 Jun 2023 11:39:43 GMT, Pavel Rappo wrote:
>> I had occasion to read over the javadoc sources in java.lang.Object recently
>> and noticed a few items that could be updated.
>>
>> There are some new or expanded API notes referring to methods in
>> java.util.Objects. I added these references as apiNote items rather than,
>> say, `@see` tags since `@see` tag changes would propagate into classes that
>> overrode the methods in questions.
>>
>> Changing toString to use an inline `@return` tag has the consequence of
>> omitting a trailing period, "." in the "Returns" section of its javadoc.
>> This also omits a trailing period in subclasses that use the `@return`
>> statement of Object.toString in their own toString method. Likewise for
>> hashCode.
>
> src/java.base/share/classes/java/lang/Object.java line 260:
>
>> 258: * }
>> 259: * The {@link java.util.Objects#toIdentityString(Object)
>> 260: * Objects.toIdentityString} method returns the string for an
>
> That last sentence doesn't feel like it needs to be part of `@implSpec`,
> although there's definitely a connection.
That is, a clear connection to what's being described by that `@implSpec`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14567#discussion_r1236873033
Re: RFR: JDK-8310453: Update javadoc of java.lang.Object
On Tue, 20 Jun 2023 17:13:26 GMT, Joe Darcy wrote: > I had occasion to read over the javadoc sources in java.lang.Object recently > and noticed a few items that could be updated. > > There are some new or expanded API notes referring to methods in > java.util.Objects. I added these references as apiNote items rather than, > say, `@see` tags since `@see` tag changes would propagate into classes that > overrode the methods in questions. > > Changing toString to use an inline `@return` tag has the consequence of > omitting a trailing period, "." in the "Returns" section of its javadoc. This > also omits a trailing period in subclasses that use the `@return` statement > of Object.toString in their own toString method. Likewise for hashCode. While it's out of scope of this PR, I note that we could also use the inline variant of `@return` in those methods of `Objects` that the `Object` refers to: `hash(Object)`, `hashCode(Object...)`, and `equals(Object, Object)`. In fact, I think they are crying for it. - PR Comment: https://git.openjdk.org/jdk/pull/14567#issuecomment-1600698509
Re: RFR: JDK-8310453: Update javadoc of java.lang.Object
On Wed, 21 Jun 2023 11:55:13 GMT, Pavel Rappo wrote: > While it's out of scope of this PR, I note that we could also use the inline > variant of `@return` in those methods of `Objects` that the `Object` refers > to: `hash(Object)`, `hashCode(Object...)`, and `equals(Object, Object)`. In > fact, I think they are crying for it. @jddarcy, I see you have since published a separate PR that includes those methods: #14608; thanks! - PR Comment: https://git.openjdk.org/jdk/pull/14567#issuecomment-1602397294
Re: RFR: JDK-8310571: Use inline @return tag on java.util.Objects
On Thu, 22 Jun 2023 00:19:03 GMT, Joe Darcy wrote:
> Small cleanup, minor differences in the wording of portions of
> toString(Object, String), nonNull(Object), requireNonNullElse, and
> requireNonNullElseGet.
I can see that `Objects.hash(Object...)` was not changed in this PR. Although I
think I can see why, I wonder if any distinctions between "Generates" and
"Returns" are not important in that context:
/**
* Generates a hash code for a sequence of input values. The hash
...
* @return a hash value of the sequence of input values
...
*/
public static int hash(Object... values) {
-
PR Comment: https://git.openjdk.org/jdk/pull/14608#issuecomment-1602410131
Re: RFR: JDK-8310571: Use inline @return tag on java.util.Objects
On Thu, 22 Jun 2023 00:19:03 GMT, Joe Darcy wrote:
> Small cleanup, minor differences in the wording of portions of
> toString(Object, String), nonNull(Object), requireNonNullElse, and
> requireNonNullElseGet.
Thanks for doing this, Joe. It seems that I have made multiple comments on the
same issue. It makes me wonder if that isn't an issue at all, but is a
deliberate choice made in this PR.
src/java.base/share/classes/java/util/Objects.java line 145:
> 143: * {@return the result of calling {@code toString} on the first
> 144: * argument if the first argument is not {@code null} and returns
> 145: * the second argument otherwise}
This "returns" in "and returns" feels non-DRY and brittle. We enclosed that
word in `{@return}` already. Maybe we could use the wording from the original
`@return` to avoid the verb altogether?
This is of course, a personal opinion.
src/java.base/share/classes/java/util/Objects.java line 251:
> 249:
> 250: /**
> 251: * {@return {@code true} if the provided reference is {@code
See my previous comment on the "returns" verb in the doc comment for
toString(Object, String). Here (IMO), you picked the better of the two options.
src/java.base/share/classes/java/util/Objects.java line 284:
> 282: /**
> 283: * {@return the first argument if it is non-{@code null} and
> 284: * otherwise returns the non-{@code null} second argument}
Same comment for "returns".
src/java.base/share/classes/java/util/Objects.java line 300:
> 298: /**
> 299: * {@return the first argument if it is non-{@code null} and
> otherwise
> 300: * returns the non-{@code null} value of {@code supplier.get()}}
Same.
-
PR Review: https://git.openjdk.org/jdk/pull/14608#pullrequestreview-1492911171
PR Review Comment: https://git.openjdk.org/jdk/pull/14608#discussion_r1238347052
PR Review Comment: https://git.openjdk.org/jdk/pull/14608#discussion_r1238367383
PR Review Comment: https://git.openjdk.org/jdk/pull/14608#discussion_r1238370870
PR Review Comment: https://git.openjdk.org/jdk/pull/14608#discussion_r1238371975
Re: RFR: JDK-8310267: Javadoc for Class#isPrimitive() is incorrect regarding Class objects for primitives
On Wed, 21 Jun 2023 00:13:24 GMT, Chen Liang wrote:
>> Correct misstatement that the Class object for a primitive type can only be
>> be access via fields like java.lang.Integer.TYPE.
>
> src/java.base/share/classes/java/lang/Class.java line 818:
>
>> 816: * they represent, namely {@code boolean}, {@code byte},
>> 817: * {@code char}, {@code short}, {@code int},
>> 818: * {@code long}, {@code float}, and {@code double}.
>
> Should we add `{@code void}` to the list here, as this is one of the
> primitive type names?
@liach please have a look at this intensive exchange between David Holmes
(@dholmes-ora) and me:
https://github.com/openjdk/jdk/pull/6213#issuecomment-960420076 That exchange
relates to other keywords that might appear in plain (i.e. prose) font.
Speaking of which, I might still owe David a follow-up PR.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14574#discussion_r1238697667
Re: RFR: 8301492: Modernize equals() method of ResourceBundle.CacheKey and Bundles.CacheKey [v3]
On Wed, 1 Feb 2023 10:36:12 GMT, Sergey Tsypanov wrote:
>> `ResourceBundle.CacheKey.equals()` and `Bundles.CacheKey.equals()` are quire
>> outdated. This simple clean-up modernizes them.
>
> Sergey Tsypanov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Restore logic
Coincidentally, I'm looking at modernizing some of the equals methods in
java.base.
src/java.base/share/classes/java/util/ResourceBundle.java line 743:
> 741: return ((module != null) &&
> (module.equals(otherEntry.getModule())) &&
> 742: (caller != null) &&
> (caller.equals(otherEntry.getCallerModule(;
> 743: } catch (NullPointerException | ClassCastException e) {
Are we sure that NPE can only be thrown when calling equals on `name` or
`locale` and not, for example, from `getModule()` or `getCallerModule()`?
-
PR Review: https://git.openjdk.org/jdk/pull/12328#pullrequestreview-1494905419
PR Review Comment: https://git.openjdk.org/jdk/pull/12328#discussion_r1239665426
Re: RFR: JDK-8310267: Javadoc for Class#isPrimitive() is incorrect regarding Class objects for primitives
On Wed, 21 Jun 2023 00:35:09 GMT, Joe Darcy wrote:
>> src/java.base/share/classes/java/lang/Class.java line 818:
>>
>>> 816: * they represent, namely {@code boolean}, {@code byte},
>>> 817: * {@code char}, {@code short}, {@code int},
>>> 818: * {@code long}, {@code float}, and {@code double}.
>>
>> Should we add `{@code void}` to the list here, as this is one of the
>> primitive type names?
>
> Hmm. I'll consider that. The javadoc in java.lang.Class is inconsistent in
> the formatting of "void" as a type name, some instances are in code markup
> while others are not.
> @jddarcy and @pavelrappo, as I understood it, @liach is not suggesting that
> the term "void" in "primitive types and void" should be replaced by `{@code
> void}`.
>
> Rather, `{@code void}` should be included in the enumeration of primitive
> types -- for example, by replacing `and {@code double}` with `{@code double},
> and {@code void}`.
AFAIK, that sentence enumerates primitive types, which according to the Java
Language Specification [^1][^2], do not include `void`.
[^1]: https://docs.oracle.com/javase/specs/jls/se20/html/jls-4.html#jls-4.2
[^2]: https://docs.oracle.com/javase/specs/jls/se20/html/jls-8.html#jls-8.4.5
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14574#discussion_r1239711787
Re: RFR: JDK-8310267: Javadoc for Class#isPrimitive() is incorrect regarding Class objects for primitives
On Fri, 23 Jun 2023 11:52:12 GMT, Sam Brannen wrote:
> Along that line of thinking, it might be best to change the first sentence to
> "Determines if the specified Class object represents a primitive type or
> void."
I was just about to suggest exactly that, but from the superinterface method's
perspective. Here's what I had been writing when received your message:
At the same, time java.lang.invoke.TypeDescriptor.OfField#isPrimitive:
/**
* Does this field descriptor describe a primitive type (including
void.)
*
* @return whether this field descriptor describes a primitive type
*/
boolean isPrimitive();
I suggest rephrasing that and similar text elsewhere, for example, as follows:
/** {@return whether this field descriptor describes a primitive type or
void} */
boolean isPrimitive();
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14574#discussion_r1239730572
Re: RFR: 8301492: Modernize equals() method of ResourceBundle.CacheKey and Bundles.CacheKey [v3]
On Fri, 23 Jun 2023 13:46:04 GMT, Sergey Tsypanov wrote:
>> src/java.base/share/classes/java/util/ResourceBundle.java line 743:
>>
>>> 741: return ((module != null) &&
>>> (module.equals(otherEntry.getModule())) &&
>>> 742: (caller != null) &&
>>> (caller.equals(otherEntry.getCallerModule(;
>>> 743: } catch (NullPointerException | ClassCastException e) {
>>
>> Are we sure that NPE can only be thrown when calling equals on `name` or
>> `locale` and not, for example, from `getModule()` or `getCallerModule()`?
>
> Yes:
>
> CacheKey(String baseName, Locale locale, Module module, Module caller) {
> Objects.requireNonNull(module);
> Objects.requireNonNull(caller);
>
> this.name = baseName;
> this.locale = locale;
> this.moduleRef = new KeyElementReference<>(module, referenceQueue, this);
> this.callerRef = new KeyElementReference<>(caller, referenceQueue, this);
> this.modulesHash = module.hashCode() ^ caller.hashCode();
> }
I'll run our CI, and if all good, I'll approve this PR. If nothing else, this
change seems reasonable and correct.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12328#discussion_r1239940961
Re: RFR: JDK-8310267: Javadoc for Class#isPrimitive() is incorrect regarding Class objects for primitives [v2]
On Fri, 23 Jun 2023 17:25:00 GMT, Joe Darcy wrote:
>> Correct misstatement that the Class object for a primitive type can only be
>> be access via fields like java.lang.Integer.TYPE.
>
> Joe Darcy has updated the pull request with a new target base due to a merge
> or a rebase. The incremental webrev excludes the unrelated changes brought in
> by the merge/rebase. The pull request contains five additional commits since
> the last revision:
>
> - Respond to review feedback.
> - Respond to review feedback.
> - Merge branch 'master' into JDK-8310267
> - Fix whitespace.
> - JDK-8310267: Javadoc for Class#isPrimitive() is incorrect regarding Class
> objects for primitives
src/java.base/share/classes/java/lang/Class.java line 823:
> 821: *
> 822: * @apiNote
> 823: * A {@code Class} object represented a primitive type can be
Not a native speaker, but "representED" not "representING"?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14574#discussion_r1240107268
Re: RFR: JDK-8310267: Javadoc for Class#isPrimitive() is incorrect regarding Class objects for primitives [v3]
On Fri, 23 Jun 2023 17:57:22 GMT, Joe Darcy wrote: >> Correct misstatement that the Class object for a primitive type can only be >> be access via fields like java.lang.Integer.TYPE. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Fix typo. Looks good to me. - Marked as reviewed by prappo (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14574#pullrequestreview-1495641556
