Re: RFR: JDK-8288368: simplify code in ValueTaglet, remove redundant code
On Mon, 13 Jun 2022 23:57:45 GMT, Jonathan Gibbons wrote: > Please review an almost trivial cleanup change, to eliminate the call to an > ill-considered method in Utils, and then remove the method itself. > > Verified that there is no significant change in the generated docs as a > result off the change. (Just changes in timestamp/version info). I assume that all jdk.javadoc tests still pass and the JDK API Documentation still builds, right? src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ValueTaglet.java line 123: > 121: return writer.valueTagOutput(field, > 122: text, > 123: field != holder); Shouldn't we use this instead (as suggested by `javax.lang.model.element.Element.equals`)? Suggestion: !field.equals(holder)); That said, I still wonder where the original author saw _inaccuracies_ with `equals`. Have you investigated how that comment came to be? - PR: https://git.openjdk.org/jdk/pull/9149
Re: RFR: JDK-8281445: Document the default value for the override-methods option
On Fri, 10 Jun 2022 02:59:29 GMT, Jonathan Gibbons wrote: > Please refer a trivial update for the javadoc command-line help, to specify > the default value for the --override-methods option. Marked as reviewed by prappo (Reviewer). - PR: https://git.openjdk.org/jdk19/pull/2
Integrated: 8287333: Clean up ParamTaglet and ThrowsTaglet
On Wed, 25 May 2022 15:39:10 GMT, Pavel Rappo wrote:
> A cleanup to facilitate fixing bugs like JDK-6509045: `{@inheritDoc}` only
> copies one instance of the specified exception.
This pull request has now been integrated.
Changeset: 024a240e
Author: Pavel Rappo
URL:
https://git.openjdk.java.net/jdk/commit/024a240e1b938504a0e8ac2acdee78d89b5a3cec
Stats: 422 lines in 4 files changed: 116 ins; 152 del; 154 mod
8287333: Clean up ParamTaglet and ThrowsTaglet
Reviewed-by: jjg
-
PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v6]
On Tue, 7 Jun 2022 20:36:26 GMT, Pavel Rappo wrote:
>> A cleanup to facilitate fixing bugs like JDK-6509045: `{@inheritDoc}` only
>> copies one instance of the specified exception.
>
> Pavel Rappo has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 37 commits:
>
> - Merge branch 'master' into 8287333
> - Address feedback
>
>Replaces String with Integer in position map. In contrast with
> ThrowsTaglet, using String instead of a more suitable type for Input.tagId is
> not a lossy transformation. So it can still be considered as cleanup. But
> generally it should be dealt with in 8285488.
> - Merge branch 'master' into 8287333
>
>It's two days before RDP1, the codebase changes quickly. This merge is to
> keep up with it.
> - Address feedback
>
>Also removes unneeded import.
> - Merge branch 'master' into 8287333
>
>This resolves a conflict in ParamTaglet.
> - Clean up if-branch
> - Remove upper-bounded wildcard
>
>This change simplifies code without any disadvantages:
>
> * Those `List` are read-only
> * An argument of the `List` type can still be passed to a `List extends XTree>` parameter
> - Simplify inheritThrowsDocumentation
> - Reuse more specific variable
> - Merge branch 'master' into 8287333
> - ... and 27 more:
> https://git.openjdk.java.net/jdk/compare/1aa87e00...418d07b5
In a mature system such as jdk.javadoc, a cleanup is a careful and continuous
process. I'll integrate what has been approved and continue cleanup in JDK 20.
-
PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v6]
On Tue, 7 Jun 2022 13:38:48 GMT, Pavel Rappo wrote: >> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java >> line 94: >> >>> 92: ? ee.getTypeParameters() >>> 93: : ee.getParameters(); >>> 94: String target = ch.getParameterName((ParamTree) >>> input.docTreeInfo.docTree()); >> >> I'm guessing that it is not practical to have DocTreeInfo be generic in the >> DocTree field. >> >> An alternative coding style here is to pass in an argument for the expected >> return type, as in >> >> String target = >> ch.getParameterName(input.docTreeInfo.docTree(ParamTree.class)); > > That relates to the plan of dealing with `Input.tagId` that we discussed > (chronologically) earlier in this PR. Since it's second time we touch this > area, here are some bits of that plan. > > Rather than generify `DocTreeInfo` and, therefore `DocFinder.Input`, we could > first note a few things about `Input`: > > * `tagId` and `docTreeInfo` are used only by `ParamTaglet` and > `ThrowsTaglet` > * `ParamTaglet` and `ThrowsTaglet` use `tagId` and `docTreeInfo` as poor > surrogates for more specific types: > * `tagId` should be `Integer` for `ParamTaglet` and `TypeMirror` for > `ThrowsTaglet` > * `Input.docTreeInfo.docTree` should be of different subtypes of > `DocTree` (as you note) > * `isTypeVariableParamTag` is used only by `ParamTaglet` > > Similarly, we note this for `Output`: `tagList` is only used by > `ThrowsTaglet`. > > What does it tell us? Well, to me it sounds like `Input` and `Output` are > nonspecific bags of values used by all inheritable tags. That needlessly > clutters `DocFinder`, which does not use *any* of those fields. > > To declutter `DocFinder` and isolate inheritable taglets from each other, a > subclass of inheritable taglet should instead use its own subclasses of > `Input` and `Output` to pass all the necessary inheritance information. > > I started working on that in JDK 19, didn't have enough time to finish. I > plan to continue working on that in JDK 20, where the result will hopefully > land. The plan is to address that in JDK-8285488. - PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v6]
On Tue, 7 Jun 2022 06:14:13 GMT, Jonathan Gibbons wrote: > Noted about tagId. You could still do a local cleanup for the type of the > map, but I guess we'll wait and see what your plan is. I cleaned up locally in 453ecbe. Mind you, the "deeper refactoring" is tracked by JDK-8285488. - PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v6]
> A cleanup to facilitate fixing bugs like JDK-6509045: `{@inheritDoc}` only
> copies one instance of the specified exception.
Pavel Rappo has updated the pull request with a new target base due to a merge
or a rebase. The pull request now contains 37 commits:
- Merge branch 'master' into 8287333
- Address feedback
Replaces String with Integer in position map. In contrast with ThrowsTaglet,
using String instead of a more suitable type for Input.tagId is not a lossy
transformation. So it can still be considered as cleanup. But generally it
should be dealt with in 8285488.
- Merge branch 'master' into 8287333
It's two days before RDP1, the codebase changes quickly. This merge is to
keep up with it.
- Address feedback
Also removes unneeded import.
- Merge branch 'master' into 8287333
This resolves a conflict in ParamTaglet.
- Clean up if-branch
- Remove upper-bounded wildcard
This change simplifies code without any disadvantages:
* Those `List` are read-only
* An argument of the `List` type can still be passed to a `List` parameter
- Simplify inheritThrowsDocumentation
- Reuse more specific variable
- Merge branch 'master' into 8287333
- ... and 27 more: https://git.openjdk.java.net/jdk/compare/1aa87e00...418d07b5
-
Changes: https://git.openjdk.java.net/jdk/pull/8886/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8886=05
Stats: 422 lines in 4 files changed: 116 ins; 152 del; 154 mod
Patch: https://git.openjdk.java.net/jdk/pull/8886.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/8886/head:pull/8886
PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v5]
On Mon, 6 Jun 2022 20:19:09 GMT, Jonathan Gibbons wrote: >> Pavel Rappo has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 35 commits: >> >> - Merge branch 'master' into 8287333 >> >>It's two days before RDP1, the codebase changes quickly. This merge is to >> keep up with it. >> - Address feedback >> >>Also removes unneeded import. >> - Merge branch 'master' into 8287333 >> >>This resolves a conflict in ParamTaglet. >> - Clean up if-branch >> - Remove upper-bounded wildcard >> >>This change simplifies code without any disadvantages: >> >> * Those `List` are read-only >> * An argument of the `List` type can still be passed to a >> `List` parameter >> - Simplify inheritThrowsDocumentation >> - Reuse more specific variable >> - Merge branch 'master' into 8287333 >> - Incremental update >> >>- Renames local variables and method parameters >>- Improves comments >>- Removes debug leftovers >> - Update top-level doc comment >> - ... and 25 more: >> https://git.openjdk.java.net/jdk/compare/f1dd559e...9c91efb0 > > src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java > line 94: > >> 92: ? ee.getTypeParameters() >> 93: : ee.getParameters(); >> 94: String target = ch.getParameterName((ParamTree) >> input.docTreeInfo.docTree()); > > I'm guessing that it is not practical to have DocTreeInfo be generic in the > DocTree field. > > An alternative coding style here is to pass in an argument for the expected > return type, as in > > String target = > ch.getParameterName(input.docTreeInfo.docTree(ParamTree.class)); That relates to the plan of dealing with `Input.tagId` that we discussed (chronologically) earlier in this PR. Since it's second time we touch this area, here are some bits of that plan. Rather than generify `DocTreeInfo` and, therefore `DocFinder.Input`, we could first note a few things about `Input`: * `tagId` and `docTreeInfo` are used only by `ParamTaglet` and `ThrowsTaglet` * `ParamTaglet` and `ThrowsTaglet` use `tagId` and `docTreeInfo` as poor surrogates for more specific types: * `tagId` should be `Integer` for `ParamTaglet` and `TypeMirror` for `ThrowsTaglet` * `Input.docTreeInfo.docTree` should be of different subtypes of `DocTree` (as you note) * `isTypeVariableParamTag` is used only by `ParamTaglet` Similarly, we note this for `Output`: `tagList` is only used by `ThrowsTaglet`. What does it tell us? Well, to me it sounds like `Input` and `Output` are nonspecific bags of values used by all inheritable tags. That needlessly clutters `DocFinder`, which does not use *any* of those fields. To declutter `DocFinder` and isolate inheritable taglets from each other, a subclass of inheritable taglet should instead use its own subclasses of `Input` and `Output` to pass all the necessary inheritance information. I started working on that in JDK 19, didn't have enough time to finish. I plan to continue working on that in JDK 20, where the result will hopefully land. - PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v5]
On Tue, 7 Jun 2022 05:45:54 GMT, Jonathan Gibbons wrote:
>> Hm... Which kind of "Error" would be thrown for which "other type"? I don't
>> think that this method is ever called with an element other than a
>> constructor or method; look:
>>
>> public ThrowsTaglet() {
>> super(DocTree.Kind.THROWS, false, EnumSet.of(Location.CONSTRUCTOR,
>> Location.METHOD));
>> }
>>
>> It is called for constructors regularly, but as you might imagine, it has no
>> effect other than waste of resources and, what is more important, loss of
>> code clarity. There are multiple types of executable elements. It's both
>> clarifying and reassuring to see an assertion that establishes our
>> programming assumptions as to which of those types we expect there, early in
>> the method.
>
> Agreed on checking stuff up front, but I continue to think that `assert` is
> little better than a stylized comment, since they are typically not enabled.
I use assertions. If you don't find them useful, we can discuss it. Last time
we discussed assertions internally, somebody sent me this article, which you
might want to have a look at: https://blog.regehr.org/archives/1091.
-
PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v5]
On Mon, 6 Jun 2022 20:40:18 GMT, Jonathan Gibbons wrote:
>> Pavel Rappo has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 35 commits:
>>
>> - Merge branch 'master' into 8287333
>>
>>It's two days before RDP1, the codebase changes quickly. This merge is to
>> keep up with it.
>> - Address feedback
>>
>>Also removes unneeded import.
>> - Merge branch 'master' into 8287333
>>
>>This resolves a conflict in ParamTaglet.
>> - Clean up if-branch
>> - Remove upper-bounded wildcard
>>
>>This change simplifies code without any disadvantages:
>>
>> * Those `List` are read-only
>> * An argument of the `List` type can still be passed to a
>> `List` parameter
>> - Simplify inheritThrowsDocumentation
>> - Reuse more specific variable
>> - Merge branch 'master' into 8287333
>> - Incremental update
>>
>>- Renames local variables and method parameters
>>- Improves comments
>>- Removes debug leftovers
>> - Update top-level doc comment
>> - ... and 25 more:
>> https://git.openjdk.java.net/jdk/compare/f1dd559e...9c91efb0
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java
> line 281:
>
>> 279: * @param namethe name of the parameter. We can't rely
>> on
>> 280: *the name in the param tag because we
>> might be
>> 281: *inheriting documentation.
>
> This is useful info you are deleting :-(
It might be better if we agreed (to be discussed and decided separately, of
course) on commenting only the stuff that requires comments. Not repetitive
stuff whose meaning is very clear, and commenting which would be boring,
useless and would add bulk. Otherwise, we might set up jdk.javadoc for more
_comment skew_, which it already has more than its fair share of.
Take this for example:
@param writer the taglet writer for output writing
What else would a method use such a parameter for? If we ever find an
alternative use for a taglet writer, not only would I accept comments, I might
ask for them.
Or this:
@param kindthe kind of param tag
I cannot imagine any other description for such a method parameter. (Being
pedantic, note that the comment doesn't explain which parameter that kind
refers to: code readers somehow guess correctly.)
Not a requirement, bible or anything, but something you might want to have a
look at: Chapter 4: Comments in Robert Cecil Martin's "Clean Code: A Handbook
of Agile Software Craftsmanship" (2008).
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ThrowsTaglet.java
> line 56:
>
>> 54:
>> 55: /**
>> 56: * A taglet that processes {@link ThrowsTree}, which represents tags like
>
> rephrase without "like"
Fixed in 195f093ec55.
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ThrowsTaglet.java
> line 112:
>
>> 110: Content result = writer.getOutputInstance();
>> 111: Set alreadyDocumented = new HashSet<>();
>> 112: result.add(throwsTagsOutput(tagsMap, writer, alreadyDocumented,
>> typeSubstitutions, true));
>
> I presume `throwsTagsOutput` does not include the header...?
Do you mean "Throws:"? If so, then it does:
if (alreadyDocumented.isEmpty()) {
result.add(writer.getThrowsHeader());
}
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/CommentHelper.java
> line 552:
>
>> 550: public ReferenceTree getExceptionName(ThrowsTree tt) {
>> 551: return tt.getExceptionName();
>> 552: }
>
> Is this method worth it?
Maybe not. I left it there because of the similar `Utils.getParameterName`
method, which is a tad bit more complicated:
public String getParameterName(ParamTree p) {
return p.getName().getName().toString();
}
-
PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v5]
> A cleanup to facilitate fixing bugs like JDK-6509045: `{@inheritDoc}` only
> copies one instance of the specified exception.
Pavel Rappo has updated the pull request with a new target base due to a merge
or a rebase. The pull request now contains 35 commits:
- Merge branch 'master' into 8287333
It's two days before RDP1, the codebase changes quickly. This merge is to
keep up with it.
- Address feedback
Also removes unneeded import.
- Merge branch 'master' into 8287333
This resolves a conflict in ParamTaglet.
- Clean up if-branch
- Remove upper-bounded wildcard
This change simplifies code without any disadvantages:
* Those `List` are read-only
* An argument of the `List` type can still be passed to a `List` parameter
- Simplify inheritThrowsDocumentation
- Reuse more specific variable
- Merge branch 'master' into 8287333
- Incremental update
- Renames local variables and method parameters
- Improves comments
- Removes debug leftovers
- Update top-level doc comment
- ... and 25 more: https://git.openjdk.java.net/jdk/compare/f1dd559e...9c91efb0
-
Changes: https://git.openjdk.java.net/jdk/pull/8886/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8886=04
Stats: 419 lines in 4 files changed: 113 ins; 152 del; 154 mod
Patch: https://git.openjdk.java.net/jdk/pull/8886.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/8886/head:pull/8886
PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v4]
On Mon, 6 Jun 2022 20:43:11 GMT, Jonathan Gibbons wrote:
>> Pavel Rappo has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 33 commits:
>>
>> - Merge branch 'master' into 8287333
>>
>>This resolves a conflict in ParamTaglet.
>> - Clean up if-branch
>> - Remove upper-bounded wildcard
>>
>>This change simplifies code without any disadvantages:
>>
>> * Those `List` are read-only
>> * An argument of the `List` type can still be passed to a
>> `List` parameter
>> - Simplify inheritThrowsDocumentation
>> - Reuse more specific variable
>> - Merge branch 'master' into 8287333
>> - Incremental update
>>
>>- Renames local variables and method parameters
>>- Improves comments
>>- Removes debug leftovers
>> - Update top-level doc comment
>> - Trivially re-order assignments
>>
>>...for re-use
>> - Reformat for clarity
>>
>>Now it's very clear that the "Throws:" section consists of three types of
>> exceptions:
>>
>> 1. documented
>> 2. inherited
>> 3. undocumented
>> - ... and 23 more:
>> https://git.openjdk.java.net/jdk/compare/ebc012ec...6bbe871b
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java
> line 149:
>
>> 147: * Tries to inherit the param tags that are missing.
>> 148: */
>> 149: private Content getTagletOutput(Element holder, ParamKind kind,
>
> It's not wrong, but I don't see the reason for rearranging the parameter list
That way the parameter kind is adjacent to parameters themselves in the list of
arguments:
ParamKind kind,
List tags,
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ThrowsTaglet.java
> line 206:
>
>> 204: // Of these two, only methods inherit documentation.
>> 205: // Don't waste time on constructors.
>> 206: assert holder.getKind() == ElementKind.CONSTRUCTOR :
>> holder.getKind();
>
> This is weaker than the old code, which would throw an Error on other types.
> `assert` may or may not be enabled.
Hm... Which kind of "Error" would be thrown for which "other type"? I don't
think that this method is ever called with an element other than a constructor
or method; look:
public ThrowsTaglet() {
super(DocTree.Kind.THROWS, false, EnumSet.of(Location.CONSTRUCTOR,
Location.METHOD));
}
It is called for constructors regularly, but as you might imagine, it has no
effect other than waste of resources and, what is more important, loss of code
clarity. There are multiple types of executable elements. It's both clarifying
and reassuring to see an assertion that establishes our programming assumptions
as to which of those types we expect there, early in the method.
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ThrowsTaglet.java
> line 217:
>
>> 215: if (inheritedDoc.tagList.isEmpty()) {
>> 216: input = new DocFinder.Input(utils, holder, this,
>> 217: utils.getTypeName(declaredExceptionType, true));
>
> I guess I don't understand your coding style, because elsewhere (earlier) in
> this review, you factored out sub-expressions into their own `var` variable.
It was later fixed in 40ca8088.
-
PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v3]
On Mon, 6 Jun 2022 21:27:25 GMT, Jonathan Gibbons wrote: >> Pavel Rappo has updated the pull request incrementally with three additional >> commits since the last revision: >> >> - Remove upper-bounded wildcard >> >>This change simplifies code without any disadvantages: >> >> * Those `List` are read-only >> * An argument of the `List` type can still be passed to a >> `List` parameter >> - Simplify inheritThrowsDocumentation >> - Reuse more specific variable > > src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java > line 92: > >> 90: ? utils.getTypeParamTrees(ee) >> 91: : utils.getParamTrees(ee); >> 92: List parameters = input.isTypeVariableParamTag > > Change both or neither; don't leave the code inconsistent I'm not sure why list of parameter trees have to be the same as list of elements. I can change (methods such as) `Utils#getTypeParamTrees`, but I cannot change (methods such as) `ExecutableElement#getTypeParameters` (sadly). Useless bounded wildcards clutter code. - PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v4]
On Mon, 6 Jun 2022 20:50:05 GMT, Jonathan Gibbons wrote:
>> Pavel Rappo has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 33 commits:
>>
>> - Merge branch 'master' into 8287333
>>
>>This resolves a conflict in ParamTaglet.
>> - Clean up if-branch
>> - Remove upper-bounded wildcard
>>
>>This change simplifies code without any disadvantages:
>>
>> * Those `List` are read-only
>> * An argument of the `List` type can still be passed to a
>> `List` parameter
>> - Simplify inheritThrowsDocumentation
>> - Reuse more specific variable
>> - Merge branch 'master' into 8287333
>> - Incremental update
>>
>>- Renames local variables and method parameters
>>- Improves comments
>>- Removes debug leftovers
>> - Update top-level doc comment
>> - Trivially re-order assignments
>>
>>...for re-use
>> - Reformat for clarity
>>
>>Now it's very clear that the "Throws:" section consists of three types of
>> exceptions:
>>
>> 1. documented
>> 2. inherited
>> 3. undocumented
>> - ... and 23 more:
>> https://git.openjdk.java.net/jdk/compare/ebc012ec...6bbe871b
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ThrowsTaglet.java
> line 90:
>
>> 88: output.tagList.add(tag);
>> 89: } else if (target != null && candidate != null &&
>> 90: utils.isTypeElement(candidate) &&
>> utils.isTypeElement(target) && // FIXME: can they be anything else other
>> than type elements?
>
> what about `TypeParameterElement`?
I deleted that FIXME in a later commit. Also, that particular if-branch is to
be looked at closely in JDK-8287796.
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ThrowsTaglet.java
> line 219:
>
>> 217: .map(t -> (ThrowsTree) t)
>> 218: .toList();
>> 219: ExecutableElement r =
>> declaredExceptionTags.put(inheritedTags, (ExecutableElement)
>> inheritedDoc.holder);
>
> I do not understand the need for saving the result
That was removed in a later commit, 6d51f62a.
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ThrowsTaglet.java
> line 237:
>
>> 235: Utils utils = writer.configuration().utils;
>> 236: Content result = writer.getOutputInstance();
>> 237: for (TypeMirror declaredExceptionType : declaredExceptionTypes)
>> {
>
> Two comments may have been too many, but zero is too few.
Did you see the name of that private method? Looks pretty self-descriptive to
me. I'm not sure why you think it needs any comment, but I can re-introduce it,
if you wish so.
-
PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v4]
On Mon, 6 Jun 2022 20:39:10 GMT, Jonathan Gibbons wrote:
>> Pavel Rappo has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 33 commits:
>>
>> - Merge branch 'master' into 8287333
>>
>>This resolves a conflict in ParamTaglet.
>> - Clean up if-branch
>> - Remove upper-bounded wildcard
>>
>>This change simplifies code without any disadvantages:
>>
>> * Those `List` are read-only
>> * An argument of the `List` type can still be passed to a
>> `List` parameter
>> - Simplify inheritThrowsDocumentation
>> - Reuse more specific variable
>> - Merge branch 'master' into 8287333
>> - Incremental update
>>
>>- Renames local variables and method parameters
>>- Improves comments
>>- Removes debug leftovers
>> - Update top-level doc comment
>> - Trivially re-order assignments
>>
>>...for re-use
>> - Reformat for clarity
>>
>>Now it's very clear that the "Throws:" section consists of three types of
>> exceptions:
>>
>> 1. documented
>> 2. inherited
>> 3. undocumented
>> - ... and 23 more:
>> https://git.openjdk.java.net/jdk/compare/ebc012ec...6bbe871b
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java
> line 147:
>
>> 145: /**
>> 146: * Returns a {@code Content} representation of a list of {@code
>> ParamTree}.
>> 147: * Tries to inherit the param tags that are missing.
>
> mild grumble to see information being removed, even it is obvious to you.
I can revert that, if you want.
-
PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v4]
On Mon, 6 Jun 2022 20:27:07 GMT, Jonathan Gibbons wrote:
> It does more than just warn ...
What else beyond converting `ParamTree`s to `Content` and warning authors about
unexpected `ParamTree` does it do?
Separately. Could it be that you haven't seen further commits? GitHub does not
seem to indicate to me which commits your comments refer to.
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java
> line 209:
>
>> 207: List
>> formalParameters,
>> 208: TagletWriter writer) {
>> 209: Map tagOfPosition = new HashMap<>();
>
> I still think that one of the type parameters should be `Integer`
See my earlier comment on `Integer`, `tagId` and `ThrowsTaglet`.
-
PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v4]
On Mon, 6 Jun 2022 20:24:08 GMT, Jonathan Gibbons wrote: >> Pavel Rappo has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 33 commits: >> >> - Merge branch 'master' into 8287333 >> >>This resolves a conflict in ParamTaglet. >> - Clean up if-branch >> - Remove upper-bounded wildcard >> >>This change simplifies code without any disadvantages: >> >> * Those `List` are read-only >> * An argument of the `List` type can still be passed to a >> `List` parameter >> - Simplify inheritThrowsDocumentation >> - Reuse more specific variable >> - Merge branch 'master' into 8287333 >> - Incremental update >> >>- Renames local variables and method parameters >>- Improves comments >>- Removes debug leftovers >> - Update top-level doc comment >> - Trivially re-order assignments >> >>...for re-use >> - Reformat for clarity >> >>Now it's very clear that the "Throws:" section consists of three types of >> exceptions: >> >> 1. documented >> 2. inherited >> 3. undocumented >> - ... and 23 more: >> https://git.openjdk.java.net/jdk/compare/ebc012ec...6bbe871b > > src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java > line 177: > >> 175: Content result = writer.getOutputInstance(); >> 176: Input input = new DocFinder.Input(writer.configuration().utils, >> holder, this, >> 177: Integer.toString(position), kind == >> ParamKind.TYPE_PARAMETER); > > Inconsistent w.r.t line 76 ... `String.valueOf` vs `Integer.toString` Huh? 02021a27 changed all `String.valueOf` to `Integer.toString` for consistency. - PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v4]
On Mon, 6 Jun 2022 20:22:34 GMT, Jonathan Gibbons wrote:
>> Pavel Rappo has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 33 commits:
>>
>> - Merge branch 'master' into 8287333
>>
>>This resolves a conflict in ParamTaglet.
>> - Clean up if-branch
>> - Remove upper-bounded wildcard
>>
>>This change simplifies code without any disadvantages:
>>
>> * Those `List` are read-only
>> * An argument of the `List` type can still be passed to a
>> `List` parameter
>> - Simplify inheritThrowsDocumentation
>> - Reuse more specific variable
>> - Merge branch 'master' into 8287333
>> - Incremental update
>>
>>- Renames local variables and method parameters
>>- Improves comments
>>- Removes debug leftovers
>> - Update top-level doc comment
>> - Trivially re-order assignments
>>
>>...for re-use
>> - Reformat for clarity
>>
>>Now it's very clear that the "Throws:" section consists of three types of
>> exceptions:
>>
>> 1. documented
>> 2. inherited
>> 3. undocumented
>> - ... and 23 more:
>> https://git.openjdk.java.net/jdk/compare/ebc012ec...6bbe871b
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java
> line 70:
>
>> 68: */
>> 69: private static Map mapNameToPosition(Utils utils,
>> List params) {
>> 70: Map result = new HashMap<>();
>
> Is there a reason not to change this to `Map`
We could change it to `Integer` now, but it wouldn't help us much: the
`DocFinder.Input.tagId` field, which that `Integer` is ultimately compared
against, is of type `String`. To make it all the way, we would need to change
`tagId` to `Integer` too. But that won't work because `tagId` is also used by
`ThrowsTaglet` to store an exception class (or an exception type variable) name.
That said, I agree that we should change `String` to `Integer` there. Let's
just not do it now as it requires deeper refactoring; I have a plan already and
some implementation in the works.
-
PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v4]
> A cleanup to facilitate fixing bugs like JDK-6509045: `{@inheritDoc}` only
> copies one instance of the specified exception.
Pavel Rappo has updated the pull request with a new target base due to a merge
or a rebase. The pull request now contains 33 commits:
- Merge branch 'master' into 8287333
This resolves a conflict in ParamTaglet.
- Clean up if-branch
- Remove upper-bounded wildcard
This change simplifies code without any disadvantages:
* Those `List` are read-only
* An argument of the `List` type can still be passed to a `List` parameter
- Simplify inheritThrowsDocumentation
- Reuse more specific variable
- Merge branch 'master' into 8287333
- Incremental update
- Renames local variables and method parameters
- Improves comments
- Removes debug leftovers
- Update top-level doc comment
- Trivially re-order assignments
...for re-use
- Reformat for clarity
Now it's very clear that the "Throws:" section consists of three types of
exceptions:
1. documented
2. inherited
3. undocumented
- ... and 23 more: https://git.openjdk.java.net/jdk/compare/ebc012ec...6bbe871b
-
Changes: https://git.openjdk.java.net/jdk/pull/8886/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8886=03
Stats: 418 lines in 4 files changed: 113 ins; 151 del; 154 mod
Patch: https://git.openjdk.java.net/jdk/pull/8886.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/8886/head:pull/8886
PR: https://git.openjdk.java.net/jdk/pull/8886
Integrated: 8287753: [spelling] close well-established compounds
On Thu, 2 Jun 2022 22:48:56 GMT, Pavel Rappo wrote: > This is a spelling and terminology PR, not a code PR. Rationale for some of > the changes are as follows: > > * "Subsignature" is mentioned in JLS 8.4.2 Method Signature. > * lower- and upper- bounded wrt `super` and `extends` are mentioned in JLS > 4.10.5 Type Projections. > * For "super super class" I used a great-grandparent analogy as I'm unaware > of any rules on repeated prefixes in English. > * "subkind" is used on two more occasions a few lines above that change This pull request has now been integrated. Changeset: a6fc485a Author:Pavel Rappo URL: https://git.openjdk.java.net/jdk/commit/a6fc485a22484b70daf170e981432c0856b9d93d Stats: 40 lines in 18 files changed: 0 ins; 0 del; 40 mod 8287753: [spelling] close well-established compounds Reviewed-by: jjg - PR: https://git.openjdk.java.net/jdk/pull/9007
Re: RFR: JDK-8252717: Integrate/merge legacy standard doclet diagnostics and doclint
On Thu, 2 Jun 2022 20:59:26 GMT, Jonathan Gibbons wrote: > Please review a moderate change to enable (most) doclet warnings even if > doclint is enabled. > > Since the introduction of doclint, there was some (small) overlap between the > small set of warnings generated by the doclet and the new larger set of > diagnostics that could be generated by doclint. The solution, until now, has > been to disable doclet warnings when doclint is enabled. But, the sets do not > overlap, and the policy has inappropriately suppressed some warnings and > inhibited the generation of new warnings by the doclet that could only be > done by the doclet, and not doclint. > > One notable group of warnings that has been inappropriately suppressed is the > warnings generated by using the `-serial warn` option. > > The fundamental core of the change is to remove the conditional checks in the > doclet `Messages.java`, which would suppress messages when doclint was > enabled. A consequence is that some specific messages have to disabled if > they are duplicate checks if doclint is enabled. (The messages cannot be > removed altogether because doclint might _not_ be enabled.) A test is added > to verify that either one message or the other is generated, but never both. > > As previously noted, an issue with the earlier policy is that warnings > generated by using the `-serial warn` option were inappropriately suppressed, > and this change fixes that ... revealing a number of latent warnings in the > JDK API that need to be addressed. The short term fix here is to temporarily > remove the use of the `-serialwarn` option. See > [JDK-8287749](https://bugs.openjdk.java.net/browse/JDK-8287749). Looks good. > I did a visual inspection over the Checker class. > The test is set up to be a framework to accommodate new cases if any should > arise. This is good. It would be even better if we had a mechanism to reduce risk of diagnostic duplication in the future. I don't have any concrete proposal, but unique internal error codes shared between doclint and doclet come to mind. - Marked as reviewed by prappo (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/9006
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v3]
> A cleanup to facilitate fixing bugs like JDK-6509045: `{@inheritDoc}` only
> copies one instance of the specified exception.
Pavel Rappo has updated the pull request incrementally with three additional
commits since the last revision:
- Remove upper-bounded wildcard
This change simplifies code without any disadvantages:
* Those `List` are read-only
* An argument of the `List` type can still be passed to a `List` parameter
- Simplify inheritThrowsDocumentation
- Reuse more specific variable
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/8886/files
- new: https://git.openjdk.java.net/jdk/pull/8886/files/7f1d9324..fbd9d76a
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=8886=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk=8886=01-02
Stats: 48 lines in 3 files changed: 10 ins; 3 del; 35 mod
Patch: https://git.openjdk.java.net/jdk/pull/8886.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/8886/head:pull/8886
PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v2]
On Fri, 3 Jun 2022 11:34:52 GMT, Pavel Rappo wrote:
>> A cleanup to facilitate fixing bugs like JDK-6509045: `{@inheritDoc}` only
>> copies one instance of the specified exception.
>
> 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 28 additional
> commits since the last revision:
>
> - Merge branch 'master' into 8287333
> - Incremental update
>
>- Renames local variables and method parameters
>- Improves comments
>- Removes debug leftovers
> - Update top-level doc comment
> - Trivially re-order assignments
>
>...for re-use
> - Reformat for clarity
>
>Now it's very clear that the "Throws:" section consists of three types of
> exceptions:
>
> 1. documented
> 2. inherited
> 3. undocumented
> - Remove dead condition
>
>tagsMap is never empty by the time that check is reached: a mapping being
> put into it 3 lines above the check.
> - Remove emptiness check
>
>Why bother in this case? It only wastes indentation.
> - Remove comments
>
>Two very similar comments to a private, self-documenting method is a bit
> too much.
> - Use consistent order
>
>Use "decalred" then "instantiated".
> - Rename and clarify
>
> - Renames a parameter to the throwsTagsOutput method
> - Widens the type of a local variable
> - ... and 18 more:
> https://git.openjdk.java.net/jdk/compare/51a35060...7f1d9324
Please review this strictly cleanup change, which allowed to better prepare for
JDK-6509045 and uncovered a few latent but major issues with `ThrowsTaglet`,
which should be addressed early in JDK 20.
-
PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v2]
> A cleanup to facilitate fixing bugs like JDK-6509045: `{@inheritDoc}` only
> copies one instance of the specified exception.
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 28 additional commits since the
last revision:
- Merge branch 'master' into 8287333
- Incremental update
- Renames local variables and method parameters
- Improves comments
- Removes debug leftovers
- Update top-level doc comment
- Trivially re-order assignments
...for re-use
- Reformat for clarity
Now it's very clear that the "Throws:" section consists of three types of
exceptions:
1. documented
2. inherited
3. undocumented
- Remove dead condition
tagsMap is never empty by the time that check is reached: a mapping being
put into it 3 lines above the check.
- Remove emptiness check
Why bother in this case? It only wastes indentation.
- Remove comments
Two very similar comments to a private, self-documenting method is a bit too
much.
- Use consistent order
Use "decalred" then "instantiated".
- Rename and clarify
- Renames a parameter to the throwsTagsOutput method
- Widens the type of a local variable
- ... and 18 more: https://git.openjdk.java.net/jdk/compare/efbfef7e...7f1d9324
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/8886/files
- new: https://git.openjdk.java.net/jdk/pull/8886/files/03ec6e6f..7f1d9324
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=8886=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk=8886=00-01
Stats: 53115 lines in 681 files changed: 27116 ins; 19658 del; 6341 mod
Patch: https://git.openjdk.java.net/jdk/pull/8886.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/8886/head:pull/8886
PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: JDK-8252717: Integrate/merge legacy standard doclet diagnostics and doclint
On Thu, 2 Jun 2022 20:59:26 GMT, Jonathan Gibbons wrote: > Please review a moderate change to enable (most) doclet warnings even if > doclint is enabled. > > Since the introduction of doclint, there was some (small) overlap between the > small set of warnings generated by the doclet and the new larger set of > diagnostics that could be generated by doclint. The solution, until now, has > been to disable doclet warnings when doclint is enabled. But, the sets do not > overlap, and the policy has inappropriately suppressed some warnings and > inhibited the generation of new warnings by the doclet that could only be > done by the doclet, and not doclint. > > One notable group of warnings that has been inappropriately suppressed is the > warnings generated by using the `-serial warn` option. > > The fundamental core of the change is to remove the conditional checks in the > doclet `Messages.java`, which would suppress messages when doclint was > enabled. A consequence is that some specific messages have to disabled if > they are duplicate checks if doclint is enabled. (The messages cannot be > removed altogether because doclint might _not_ be enabled.) A test is added > to verify that either one message or the other is generated, but never both. > > As previously noted, an issue with the earlier policy is that warnings > generated by using the `-serial warn` option were inappropriately suppressed, > and this change fixes that ... revealing a number of latent warnings in the > JDK API that need to be addressed. The short term fix here is to temporarily > remove the use of the `-serialwarn` option. See > [JDK-8287749](https://bugs.openjdk.java.net/browse/JDK-8287749). > Since the introduction of doclint, there was some (small) overlap between the > small set ... and the new larger set ... But, the sets do not overlap Do or do they not overlap? :) > the policy has inappropriately suppressed some warnings and inhibited the > generation of new warnings What's the difference between "suppressed" and "inhibited" here? Generally, how can we be sure that we haven't missed any double-reported cases? test/langtools/jdk/javadoc/doclet/testDoclintDocletMessages/TestDocLintDocletMessages.java line 136: > 134: var doclintResult = new Result(Exit.OK, "C.java:8: warning: > @param \"x\" has already been specified"); > 135: var docletResult = new Result(Exit.OK, "C.java:8: warning: > Parameter \"x\" is documented more than once."); > 136: Both are warnings, hence two `Exit.OK` results; I see. test/langtools/jdk/javadoc/doclet/testDoclintDocletMessages/TestDocLintDocletMessages.java line 190: > 188: checkExit(expect.exit); > 189: > 190: checkOutput(Output.OUT, true,expect.message); Add whitespace after `true,` - PR: https://git.openjdk.java.net/jdk/pull/9006
RFR: 8287753: [spelling] close well-established compounds
This is a spelling and terminology PR, not a code PR. Rationale for some of the changes are as follows: * "Subsignature" is mentioned in JLS 8.4.2 Method Signature. * lower- and upper- bounded wrt `super` and `extends` are mentioned in JLS 4.10.5 Type Projections. * For "super super class" I used a great-grandparent analogy as I'm unaware of any rules on repeated prefixes in English. * "subkind" is used on two more occasions a few lines above that change - Commit messages: - Initial commit Changes: https://git.openjdk.java.net/jdk/pull/9007/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=9007=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8287753 Stats: 40 lines in 18 files changed: 0 ins; 0 del; 40 mod Patch: https://git.openjdk.java.net/jdk/pull/9007.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/9007/head:pull/9007 PR: https://git.openjdk.java.net/jdk/pull/9007
Re: RFR: JDK-8287606: standardize spelling of subtype and supertype etc in comments
On Wed, 1 Jun 2022 05:20:22 GMT, Jonathan Gibbons wrote:
> Please review a trivial fix to remove unnecessary hyphens from compound words
> in comments,
> as suggested in an earlier review.
Looks okay; thanks.
This PR changes hyphenated forms to closed forms. Consider also changing open
forms (e.g. sub package, super interface) to closed forms. We have some of
those in jdk.javadoc.
src/jdk.javadoc/share/classes/jdk/javadoc/doclet/Reporter.java line 52:
> 50: * such as the command-line help that is generated when using a {@code
> --help} option,
> 51: * and "diagnostic output" refers to any errors, warnings and other
> output that is
> 52: * a side effect of executing the operation.
While I'm not saying that your suggestion is incorrect (in fact, my ngram
comparison result supports you), I note that "side-effect" is a prevailing
spelling in JDK.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/builders/AbstractMemberBuilder.java
line 60:
> 58:
> 59: /**
> 60: * This method is not supported by subbuilders.
Hopefully we'll get rid of this concept and the need for that _shudders_ word
eventually.
-
Marked as reviewed by prappo (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8964
Re: RFR: JDK-8285939: javadoc java.lang.Record should not have "Direct Known Subclasses:" section [v3]
On Wed, 25 May 2022 20:55:21 GMT, Jonathan Gibbons wrote:
>> As described in the JBS issue, the observed problem is a side-effect of a
>> related but different issue, which is that record classes are not treated
>> the same was as enum classes when it comes to generating the class hierarchy
>> in `ClassTree`. Because record classes are not treated
>> specially/differently, they are treated as ordinary/plain classes, with the
>> side-effect that the page for `java.lang.Record` shows `Direct Known
>> Subclasses`.
>>
>> The underlying fix is therefore to clone the enum support in `ClassTree` and
>> related classes, to provide support for record classes. It is possible to do
>> an extreme minimal clone, but that just clones some of the messy evolution
>> already there. Instead, the code in `ClassTree` and its clients is
>> refactored and modernized.
>>
>> Previously there were four explicit pairs of member fields, containing data
>> for different groups (hierarchies) of classes, namely: plain classes, enum
>> classes, interfaces and annotation interfaces. These fields are most of the
>> code to support them are moved into some new abstractions to encapsulate
>> related functionality.
>>
>> 1. The main new abstraction in `ClassTree` is `Hierarchy` which provides the
>> support for the different hierarchies displayed in the generated pages.
>> 2. A new enum `HierarchyKind` identifies the four existing hierarchies
>> (listed above) and now a new one, for record classes. The hierarchies
>> correspond to the different kinds of declared type.
>> 3. A small new class `SubtypeMap` which is a multi-map for mapping a type
>> element to its subtypes. This is used in `Hierarchy` and to record the
>> classes that implement an interfaces.
>>
>> Generally, the naming should be clearer and more obvious. The most confusing
>> name in the old code was `enumSubtypes` which was weird because enum classes
>> don't have subtypes. It meant "subtypes of supertypes of enum classes".
>> This was a prime motivator to switch to the `hierarchy` terminology. The
>> variant spellings of `intfacs` have all been replaced by `interfaces`, and
>> `classtree` becomes `classTree`.
>>
>> *Testing*: a new test case has been added to the `TestRecordTypes.java`
>> test, which verifies the new record hierarchy is displayed on a a package
>> tree page. It is not simple to directly test the observed/reported
>> behavior, because it is specific to the JDK API documentation, and because
>> it is about the content of the `java.lang.Record` API page. However, manual
>> inspection and diff comparison between JDK API documentation generated
>> before and after this change reveals only expected differences. These
>> differences are on the `java.lang.R4cord` page (where the offending section
>> is no longer displayed) and on the pages related to the two existing records
>> in JDK ... which are now listed in `Record Class Hierarchy` sections in the
>> appropriate `package-tree.html` files.
>
> Jonathan Gibbons has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains five commits:
>
> - Merge remote-tracking branch 'upstream/master' into 8285939.record-subtypes
> - address review comments: add doc comments to new methods
> - merge with upstream master
> - fix copyright; update test description
> - JDK-8285939: javadoc java.lang.Record should not have "Direct Known
> Subclasses:" section
Looks good.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/ClassTree.java
line 78:
> 76: /**
> 77: * {@return the roots of the hierarchy}
> 78: * The roots are the classes or interfaces with no superclass or
> superinterfaces.
Use singular "root" for clarity:
Suggestion:
* A root is a class or an interface with no superclass or
superinterfaces.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/ClassTree.java
line 88:
> 86: * {@return a map containing the type elements in this hierarchy
> and their subtypes}
> 87: */
> 88: public Map> subtypes() {
This method is unused; consider deleting it.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/ClassTree.java
line 103:
> 101: * {@return the set of all subtypes of the given type element,
> or an empty set if there are none}
> 102: *
> 103: * The set of all subtypes is the transitive closure of the
> {@linkplain #subtypes() immediate subtypes}
Did you mean to link to #subtypes(TypeElement)?
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/ClassTree.java
line 128:
> 126: /**
> 127: * A map giving the subtypes for each of a collection of type
> elements.
> 128: * The subtypes may be subclasses or subinterfaces, depending on the
> context.
For a separate PR: we should consider dropping hyphens from "super-" and
Re: RFR: JDK-8277420: Provide a way to copy the hyperlink to a doc element to the clipboard [v3]
On Fri, 27 May 2022 07:16:23 GMT, Hannes Wallnöfer wrote: >> This is a CSS/JavaScript only change to implement copy-to-clipboard >> functionality for all headers (`h1` - `h6`) that have an `id` attribute >> associated with them. The following element-attribute patterns are supported >> (using `` as an example): >> >> - `` (generated by javadoc) >> - `` (commonly used) >> - `` (legacy) >> >> The change includes a consolidation of the CSS styles used to render >> copy-to-clipboard buttons, of which we have now three kinds: for snippets, >> for the link on the search page, and for headers. There is now a base CSS >> class called "copy" that defines the styles shared by all copy-to-clipboard >> buttons, and additional CSS properties for the concrete "subclasses". >> >> API docs generated with this change can be viewed here (top level files and >> java.base module): >> http://cr.openjdk.java.net/~hannesw/8277420/api.03/ > > Hannes Wallnöfer 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: > > - Update new snippet test for CSS changes > - Merge branch 'master' into JDK-8277420 > - Fix jquery selector for anchor within header > - Clean up copy-to-clipboard button styles > - JDK-8277420: Provide a way to copy the hyperlink to a doc element to the > clipboard Marked as reviewed by prappo (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8817
Re: RFR: JDK-8277420: Provide a way to copy the hyperlink to a doc element to the clipboard [v2]
On Mon, 23 May 2022 19:09:41 GMT, Hannes Wallnöfer wrote: >> This is a CSS/JavaScript only change to implement copy-to-clipboard >> functionality for all headers (`h1` - `h6`) that have an `id` attribute >> associated with them. The following element-attribute patterns are supported >> (using `` as an example): >> >> - `` (generated by javadoc) >> - `` (commonly used) >> - `` (legacy) >> >> The change includes a consolidation of the CSS styles used to render >> copy-to-clipboard buttons, of which we have now three kinds: for snippets, >> for the link on the search page, and for headers. There is now a base CSS >> class called "copy" that defines the styles shared by all copy-to-clipboard >> buttons, and additional CSS properties for the concrete "subclasses". >> >> API docs generated with this change can be viewed here (top level files and >> java.base module): >> http://cr.openjdk.java.net/~hannesw/8277420/api.03/ > > Hannes Wallnöfer has updated the pull request incrementally with one > additional commit since the last revision: > > Fix jquery selector for anchor within header Ensure TestSnippetUnnamedPackage passes before integrating. - PR: https://git.openjdk.java.net/jdk/pull/8817
Re: RFR: JDK-8277420: Provide a way to copy the hyperlink to a doc element to the clipboard [v2]
On Mon, 23 May 2022 19:09:41 GMT, Hannes Wallnöfer wrote: >> This is a CSS/JavaScript only change to implement copy-to-clipboard >> functionality for all headers (`h1` - `h6`) that have an `id` attribute >> associated with them. The following element-attribute patterns are supported >> (using `` as an example): >> >> - `` (generated by javadoc) >> - `` (commonly used) >> - `` (legacy) >> >> The change includes a consolidation of the CSS styles used to render >> copy-to-clipboard buttons, of which we have now three kinds: for snippets, >> for the link on the search page, and for headers. There is now a base CSS >> class called "copy" that defines the styles shared by all copy-to-clipboard >> buttons, and additional CSS properties for the concrete "subclasses". >> >> API docs generated with this change can be viewed here (top level files and >> java.base module): >> http://cr.openjdk.java.net/~hannesw/8277420/api.03/ > > Hannes Wallnöfer has updated the pull request incrementally with one > additional commit since the last revision: > > Fix jquery selector for anchor within header Code: * Your JavaScript code looks very readable to me, and I don't know JavaScript; nice. * Don't forget to update the copyright years. UI: * Consider using a different icon for the button that copies a link. From what I've seen, icons depicting chain links are typical ( e.g. https://graphicdesign.stackexchange.com/questions/132106/what-is-the-most-intuitive-and-obvious-copy-link-icon ) * When the header button appears, the text on the page shifts a bit. Look closely and you'll see it. Don't know if it could be fixed easily, though. Looks good. - Marked as reviewed by prappo (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8817
Re: RFR: JDK-8279513: jdk/javadoc/doclet/testDocletExample/TestDocletExample.java fails after 8278795
On Thu, 19 May 2022 22:05:50 GMT, Jonathan Gibbons wrote: > Please review a test-only fix to make a recent new test more robust. > > The test was not fundamentally broken, so the essential functionality remains > the same. However, it does assume access to the `src/jdk.javadoc` directory, > and crashed when that was not available. > > The mitigation is two-fold: > > 1. introduce and use a new `SnippetUtils.ConfigurationException` that is > thrown when the test cannot find the necessary directories. > 2. introduce and use 2 new test-suite keywords, `needs-src` > `needs-src-jdk_javadoc` that can be used on the jtreg command-line to filter > out this test, and any similar tests in the future. > > Tested locally, manually, by temporarily renaming key directories, to test > the different code paths. In all cases, if the test is run and any necessary > directories are missing, the test _will still fail_, but now with a more > useful and specific exception and detail message. > I'd prefer to leave updates to SnippetIUtils to its own separate Enhancement. Sounds good. - Marked as reviewed by prappo (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8796
Re: RFR: JDK-8279513: jdk/javadoc/doclet/testDocletExample/TestDocletExample.java fails after 8278795
On Wed, 25 May 2022 16:59:25 GMT, Jonathan Gibbons wrote:
>> test/langtools/jdk/javadoc/doclet/testDocletExample/TestDocletExample.java
>> line 71:
>>
>>> 69: var entryPointSnippet = snippets.getSnippetById(dc,
>>> "entry-point");
>>> 70: if (entryPointSnippet == null) {
>>> 71: throw new Error("Cannot find snippet \"entry-point\"");
>>
>> I understand it as follows. Although this code now throws generic `Error`
>> instead of `NullPointerException`, which the bug reporter observed, we
>> shouldn't see that `Error` in circumstances similar to those of bug
>> reporter. This is because in those circumstances the code will throw
>> `ConfigurationException` earlier, at construction time, so we won't reach
>> this check. Do I understand it correctly?
>
> Yes. Although we could throw NPE even here, I was wanting to throw something
> that indicates the test is dysfunctional, as compared to failing or crashing.
The rationale that you provided reminds of `new FileInputStream(java.io.File)`
that throws `FileNotFoundException`. Unlike not finding a file, not finding a
particular snippet is an unexpected, unrecoverable programming error. I cannot
imagine a reasonable test that checks that some snippet is not there.
-
PR: https://git.openjdk.java.net/jdk/pull/8796
Re: RFR: JDK-8279513: jdk/javadoc/doclet/testDocletExample/TestDocletExample.java fails after 8278795
On Wed, 25 May 2022 17:02:15 GMT, Jonathan Gibbons wrote: > `SnippetUtils` is broken if we get a null result. If it's unconditionally so, which it seems to be, wouldn't it be better to throw something from `SnippetUtils.getSnippetById` instead? - PR: https://git.openjdk.java.net/jdk/pull/8796
Re: RFR: JDK-8284037: Snippet-files subdirectory not automatically detected when in unnamed package [v3]
On Wed, 25 May 2022 17:00:16 GMT, Jonathan Gibbons wrote: >> Please review a small fix to address use of snippets in source code in the >> unnamed package. >> >> The core of the fix is to replace `packageName(pkg, utils)` (which returns >> `""` for the unnamed package) with plain old >> `pkg.getQualifiedName().toString()` (which returns an empty string for the >> unnamed package.) >> >> There's some minor localized cleanup and rearrangement as well. >> >> The test verifies behavior without and then with a source path, since that >> is the likely path of discovery by a user playing with the unnamed package. > > Jonathan Gibbons has updated the pull request incrementally with one > additional commit since the last revision: > > address review feedback: remove dead code Marked as reviewed by prappo (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8708
RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet
A cleanup to facilitate fixing bugs like JDK-6509045: `{@inheritDoc}` only
copies one instance of the specified exception.
-
Commit messages:
- Update top-level doc comment
- Trivially re-order assignments
- Reformat for clarity
- Remove dead condition
- Remove emptiness check
- Remove comments
- Use consistent order
- Rename and clarify
- Improve `inherit`
- Rearrange methods
- ... and 16 more: https://git.openjdk.java.net/jdk/compare/e990fec1...03ec6e6f
Changes: https://git.openjdk.java.net/jdk/pull/8886/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8886=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8287333
Stats: 382 lines in 3 files changed: 111 ins; 158 del; 113 mod
Patch: https://git.openjdk.java.net/jdk/pull/8886.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/8886/head:pull/8886
PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: JDK-6251738: Want a top-level summary page that itemizes all spec documents referenced from javadocs (OEM spec) [v5]
On Tue, 24 May 2022 19:50:41 GMT, Jonathan Gibbons wrote: >> Please review the code and tests to add support for a new `@spec url title` >> tag to javadoc. Note, this does not include any changes to API doc comments >> to use the new tag in JDK API documentation; that will come later. > > Jonathan Gibbons has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains seven commits: > > - Merge remote-tracking branch 'upstream/master' into 6251738.spec-tag-2 > - address review feedback > - fix merge issues > - merge with upstream/master > - fix whitespace and position of HtmlStyle.refList > - fix whitespace > - JDK-6251738: Want a top-level summary page that itemizes all spec > documents referenced from javadocs (OEM spec) I appreciate your perseverance in this review, Jon! A note to self: when we have the next round of review, most URIs will have likely turned into URLs. - PR: https://git.openjdk.java.net/jdk/pull/8439
Re: RFR: JDK-6251738: Want a top-level summary page that itemizes all spec documents referenced from javadocs (OEM spec) [v5]
On Tue, 24 May 2022 21:29:53 GMT, Pavel Rappo wrote:
>> Jonathan Gibbons has updated the pull request with a new target base due to
>> a merge or a rebase. The pull request now contains seven commits:
>>
>> - Merge remote-tracking branch 'upstream/master' into 6251738.spec-tag-2
>> - address review feedback
>> - fix merge issues
>> - merge with upstream/master
>> - fix whitespace and position of HtmlStyle.refList
>> - fix whitespace
>> - JDK-6251738: Want a top-level summary page that itemizes all spec
>> documents referenced from javadocs (OEM spec)
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/BaseOptions.java
> line 302:
>
>> 300:
>> 301: /**
>> 302: * Argument for command-line option {@code --spec-base-URI}.
>
> Have my earlier comment on the upper case in `--spec-base-URI` disappeared? I
> cannot seem to find it.
Ah, found it: there are two of those cases.
-
PR: https://git.openjdk.java.net/jdk/pull/8439
Re: RFR: JDK-6251738: Want a top-level summary page that itemizes all spec documents referenced from javadocs (OEM spec) [v5]
On Tue, 24 May 2022 19:50:41 GMT, Jonathan Gibbons wrote:
>> Please review the code and tests to add support for a new `@spec url title`
>> tag to javadoc. Note, this does not include any changes to API doc comments
>> to use the new tag in JDK API documentation; that will come later.
>
> Jonathan Gibbons has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains seven commits:
>
> - Merge remote-tracking branch 'upstream/master' into 6251738.spec-tag-2
> - address review feedback
> - fix merge issues
> - merge with upstream/master
> - fix whitespace and position of HtmlStyle.refList
> - fix whitespace
> - JDK-6251738: Want a top-level summary page that itemizes all spec
> documents referenced from javadocs (OEM spec)
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/BaseOptions.java
line 302:
> 300:
> 301: /**
> 302: * Argument for command-line option {@code --spec-base-URI}.
Have my earlier comment on the upper case in `--spec-base-URI` disappeared? I
cannot seem to find it.
-
PR: https://git.openjdk.java.net/jdk/pull/8439
Re: RFR: JDK-8284037: Snippet-files subdirectory not automatically detected when in unnamed package [v2]
On Mon, 16 May 2022 23:16:25 GMT, Jonathan Gibbons wrote: >> Please review a small fix to address use of snippets in source code in the >> unnamed package. >> >> The core of the fix is to replace `packageName(pkg, utils)` (which returns >> `""` for the unnamed package) with plain old >> `pkg.getQualifiedName().toString()` (which returns an empty string for the >> unnamed package.) >> >> There's some minor localized cleanup and rearrangement as well. >> >> The test verifies behavior without and then with a source path, since that >> is the likely path of discovery by a user playing with the unnamed package. > > Jonathan Gibbons has updated the pull request incrementally with one > additional commit since the last revision: > > address review feedback Changes requested by prappo (Reviewer). src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/SnippetTaglet.java line 223: > 221: JavaFileManager.Location l = > utils.getLocationForPackage(pkg); > 222: String relativeName = "snippet-files/" + v; > 223: String packageName = packageName(pkg, utils); Drop the now-unused `packageName` method. - PR: https://git.openjdk.java.net/jdk/pull/8708
Re: RFR: JDK-8284037: Snippet-files subdirectory not automatically detected when in unnamed package [v2]
On Mon, 16 May 2022 22:54:55 GMT, Jonathan Gibbons wrote: >> Going forward, how about an `Args` builder, with fluent methods >> `add(String)`, `addAll(String...)`, `addAll(List)` and >> `add(Optional)` ... although that last one will cause warnings from >> javac which grumbles about using `Optional` as a parameter type. >> >> Instead of using `Optional`, we could have overloads `addIf(boolean, >> String)` etc. >> >> I'm open to suggestions for a better/neater methodology here. > > [JDK-8286842](https://bugs.openjdk.java.net/browse/JDK-8286842) Args builder sounds reasonable. - PR: https://git.openjdk.java.net/jdk/pull/8708
Re: RFR: JDK-8286101: Support formatting in @value tag [v3]
On Tue, 17 May 2022 23:14:48 GMT, Jonathan Gibbons wrote:
>> This PR is for an update to the doc comment `{@value}` tag to permit an
>> optional [format
>> string](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/Formatter.html#syntax)
>> after the name of the tag.
>>
>> {@value optional-format-string optional-reference}
>>
>> If given, the format string should either begin with `%` or the string
>> should be quoted with `"`. For example, `%4x` or `"0x%4x"`. The format
>> string must contain exactly one `%` character.
>
> Jonathan Gibbons 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 six additional
> commits since the last revision:
>
> - fix typo
> - Merge remote-tracking branch 'upstream/master' into 8286101.format-at-value
> - address review feedback
> - address review feedback
> - Merge remote-tracking branch 'upstream/master' into 8286101.format-at-value
> - JDK-8286101: Support formatting in @value tag
Marked as reviewed by prappo (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/8565
Re: RFR: JDK-8279513: jdk/javadoc/doclet/testDocletExample/TestDocletExample.java fails after 8278795
On Thu, 19 May 2022 22:05:50 GMT, Jonathan Gibbons wrote:
> Please review a test-only fix to make a recent new test more robust.
>
> The test was not fundamentally broken, so the essential functionality remains
> the same. However, it does assume access to the `src/jdk.javadoc` directory,
> and crashed when that was not available.
>
> The mitigation is two-fold:
>
> 1. introduce and use a new `SnippetUtils.ConfigurationException` that is
> thrown when the test cannot find the necessary directories.
> 2. introduce and use 2 new test-suite keywords, `needs-src`
> `needs-src-jdk_javadoc` that can be used on the jtreg command-line to filter
> out this test, and any similar tests in the future.
>
> Tested locally, manually, by temporarily renaming key directories, to test
> the different code paths. In all cases, if the test is run and any necessary
> directories are missing, the test _will still fail_, but now with a more
> useful and specific exception and detail message.
test/langtools/jdk/javadoc/doclet/testDocletExample/TestDocletExample.java line
71:
> 69: var entryPointSnippet = snippets.getSnippetById(dc,
> "entry-point");
> 70: if (entryPointSnippet == null) {
> 71: throw new Error("Cannot find snippet \"entry-point\"");
I understand it as follows. Although this code now throws generic `Error`
instead of `NullPointerException`, which the bug reporter observed, we
shouldn't see that `Error` in circumstances similar to those of bug reporter.
This is because in those circumstances the code will throw
`ConfigurationException` earlier, at construction time, so we won't reach this
check. Do I understand it correctly?
test/langtools/jdk/javadoc/doclet/testDocletExample/TestDocletExample.java line
97:
> 95: var dc = snippets.getDocTrees().getDocCommentTree(docletPkg);
> 96: var exampleSnippet = snippets.getSnippetById(dc, "Example.java");
> 97: if (exampleSnippet == null) {
Same as above.
-
PR: https://git.openjdk.java.net/jdk/pull/8796
Re: RFR: JDK-6251738: Want a top-level summary page that itemizes all spec documents referenced from javadocs (OEM spec) [v4]
On Wed, 18 May 2022 18:43:44 GMT, Jonathan Gibbons wrote:
>> Please review the code and tests to add support for a new `@spec url title`
>> tag to javadoc. Note, this does not include any changes to API doc comments
>> to use the new tag in JDK API documentation; that will come later.
>
> Jonathan Gibbons has updated the pull request incrementally with one
> additional commit since the last revision:
>
> address review feedback
src/jdk.compiler/share/classes/com/sun/source/doctree/DocTreeVisitor.java line
302:
> 300: * @since 19
> 301: */
> 302: default R visitSpec(SpecTree node, P p) {
Nit: this method breaks the alphabetic order of visitor methods. Although
`visitOther` also breaks it, we should not address it in this PR.
src/jdk.compiler/share/classes/com/sun/source/util/DocTreeScanner.java line 515:
> 513: * {@inheritDoc}
> 514: *
> 515: * @param node {@inheritDoc}
There's something wrong with the `@implSpec` tags in this and the immediately
following two methods. Might be an artifact of the patch.
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 1533:
> 1531: public DCTree parse(int pos) throws ParseException {
> 1532: skipWhitespace();
> 1533: DCText url = inlineWord();
Nit: call it `uri` for consistency.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/resources/standard.properties
line 432:
> 430: doclet.extSpec.url.title=\
> 431: url: {0}, title: "{1}"
> 432:
Use `uri` in all 4 resources?
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/BaseOptions.java
line 1098:
> 1096:
> 1097: /**
> 1098: * Argument for command-line option {@code --spec-base-URI}.
I think the option is lower-cased `--spec-base-uri`.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/SpecTaglet.java
line 2:
> 1: /*
> 2: * Copyright (c) 2001, 2022, Oracle and/or its affiliates. All rights
> reserved.
2001?
test/langtools/tools/javac/diags/examples/NoTitle.java line 2:
> 1: /*
> 2: * Copyright (c) 2012, 2022, Oracle and/or its affiliates. All rights
> reserved.
2012? Is it an old file from the previous incarnation of the `@spec` tag?
-
PR: https://git.openjdk.java.net/jdk/pull/8439
Re: RFR: JDK-8286832: JavaDoc pages call browser history API too often
On Tue, 17 May 2022 13:48:25 GMT, Hannes Wallnöfer wrote: > This is a small but important fix in our browser script that ensures that the > `history.replaceState()` function is not called repeatedly during scrolling. > The existing code makes sure the function is called no more often then every > 100 milliseconds, but that is still unnecessarily often and enough to make > Firefox complain and degrade the scrolling performance on Chrome on Android. > > The new code does not call the function until after the user has stopped > scrolling. This is what we want and fixes all the above mentioned problems. > > I have tested the new code thoroughly on Firefox and Chrome on Mac OS and > Linux, Safari on Mac OS and iOS as well as Chrome on Android. Marked as reviewed by prappo (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8751
Re: RFR: JDK-8286887: Remove logging from search.js
On Tue, 17 May 2022 14:13:38 GMT, Hannes Wallnöfer wrote: > This is a trivial change to remove a logging statement in search.js that > sneaked in with JDK-8248863. Marked as reviewed by prappo (Reviewer). Marked as reviewed by prappo (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8752
Re: RFR: JDK-8286832: JavaDoc pages call browser history API too often
On Tue, 17 May 2022 13:48:25 GMT, Hannes Wallnöfer wrote:
> This is a small but important fix in our browser script that ensures that the
> `history.replaceState()` function is not called repeatedly during scrolling.
> The existing code makes sure the function is called no more often then every
> 100 milliseconds, but that is still unnecessarily often and enough to make
> Firefox complain and degrade the scrolling performance on Chrome on Android.
>
> The new code does not call the function until after the user has stopped
> scrolling. This is what we want and fixes all the above mentioned problems.
>
> I have tested the new code thoroughly on Firefox and Chrome on Mac OS and
> Linux, Safari on Mac OS and iOS as well as Chrome on Android.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/resources/script.js
line 142:
> 140: contentDiv.addEventListener("scroll", function(e) {
> 141: var timeoutID;
> 142: if (!timeoutID) {
I don't understand JavaScript, so could you briefly explain how that _used_ to
work? It reads like we declare a variable and then immediately check it for
being set?
-
PR: https://git.openjdk.java.net/jdk/pull/8751
Integrated: 8287099: Clean up terminology regarding doc comment descriptions.
On Fri, 20 May 2022 20:55:02 GMT, Pavel Rappo wrote: > Use the term "main description". This pull request has now been integrated. Changeset: 7c086475 Author: Pavel Rappo URL: https://git.openjdk.java.net/jdk/commit/7c0864752aa6301ec5a2123a5a96eb71bc0a83af Stats: 11 lines in 6 files changed: 0 ins; 0 del; 11 mod 8287099: Clean up terminology regarding doc comment descriptions. Reviewed-by: jjg - PR: https://git.openjdk.java.net/jdk/pull/8818
RFR: 8287099: Clean up terminology regarding doc comment descriptions.
Use the term "main description". - Commit messages: - Update copyright years - Initial commit Changes: https://git.openjdk.java.net/jdk/pull/8818/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8818=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8287099 Stats: 11 lines in 6 files changed: 0 ins; 0 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/8818.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8818/head:pull/8818 PR: https://git.openjdk.java.net/jdk/pull/8818
Re: RFR: JDK-8286101: Support formatting in @value tag
On Thu, 5 May 2022 23:39:46 GMT, Jonathan Gibbons wrote:
> This PR is for an update to the doc comment `{@value}` tag to permit an
> optional [format
> string](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/Formatter.html#syntax)
> after the name of the tag.
>
> {@value optional-format-string optional-reference}
>
> If given, the format string should either begin with `%` or the string should
> be quoted with `"`. For example, `%4x` or `"0x%4x"`. The format string must
> contain exactly one `%` character.
So far the idea looks okay. My feedback is purely on its implementation.
I'm concerned about backward compatibility. If we believe that it is unlikely
that javadoc can be configured to use a custom implentation of
`DocTreeFactory`, then the proposed implementation looks okay. Otherwise,
consider a case where a new javadoc tool reads old doc comments. Do we expect
it to fail with `UnsupportedOperationException` for every `value` tag in those
doc comments?
My inline comments below are about that latter case; ignore them if you don't
think it's an issue.
src/jdk.compiler/share/classes/com/sun/source/doctree/ValueTree.java line 55:
> 53: default TextTree getFormat() {
> 54: throw new UnsupportedOperationException();
> 55: }
Return `null` and specify this behavior in the `@implSpec` section.
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 1605:
> 1603: if (ch == '}') {
> 1604: nextChar();
> 1605: return m.at(pos).newValueTree(format, ref);
If `format == null` call `newValueTree(ref)`; otherwise call
`newValueTree(format, ref)`.
src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DocTreeMaker.java line
482:
> 480:
> 481: @Override @DefinedBy(Api.COMPILER_TREE)
> 482: public DCValue newValueTree(ReferenceTree ref) {
Revert the implementation of this method.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ValueTaglet.java
line 112:
> 110: text = String.format(configuration.getLocale(), f,
> field.getConstantValue());
> 111: } catch (IllegalFormatException e) {
> 112: messages.warning(holder,
Doesn't CSR say that it is an *error*?
test/langtools/tools/javac/doctree/ValueTest.java line 108:
> 106: */
> 107:
> 108: /**
Consider adding the following cases:
* erroneous format
* valid format and reference followed by the trailing "junk"
-
PR: https://git.openjdk.java.net/jdk/pull/8565
Re: RFR: JDK-8285939: javadoc java.lang.Record should not have "Direct Known Subclasses:" section
On Tue, 3 May 2022 20:38:44 GMT, Jonathan Gibbons wrote: > As described in the JBS issue, the observed problem is a side-effect of a > related but different issue, which is that record classes are not treated the > same was as enum classes when it comes to generating the class hierarchy in > `ClassTree`. Because record classes are not treated specially/differently, > they are treated as ordinary/plain classes, with the side-effect that the > page for `java.lang.Record` shows `Direct Known Subclasses`. > > The underlying fix is therefore to clone the enum support in `ClassTree` and > related classes, to provide support for record classes. It is possible to do > an extreme minimal clone, but that just clones some of the messy evolution > already there. Instead, the code in `ClassTree` and its clients is refactored > and modernized. > > Previously there were four explicit pairs of member fields, containing data > for different groups (hierarchies) of classes, namely: plain classes, enum > classes, interfaces and annotation interfaces. These fields are most of the > code to support them are moved into some new abstractions to encapsulate > related functionality. > > 1. The main new abstraction in `ClassTree` is `Hierarchy` which provides the > support for the different hierarchies displayed in the generated pages. > 2. A new enum `HierarchyKind` identifies the four existing hierarchies > (listed above) and now a new one, for record classes. The hierarchies > correspond to the different kinds of declared type. > 3. A small new class `SubtypeMap` which is a multi-map for mapping a type > element to its subtypes. This is used in `Hierarchy` and to record the > classes that implement an interfaces. > > Generally, the naming should be clearer and more obvious. The most confusing > name in the old code was `enumSubtypes` which was weird because enum classes > don't have subtypes. It meant "subtypes of supertypes of enum classes". This > was a prime motivator to switch to the `hierarchy` terminology. The variant > spellings of `intfacs` have all been replaced by `interfaces`, and > `classtree` becomes `classTree`. > > *Testing*: a new test case has been added to the `TestRecordTypes.java` test, > which verifies the new record hierarchy is displayed on a a package tree > page. It is not simple to directly test the observed/reported behavior, > because it is specific to the JDK API documentation, and because it is about > the content of the `java.lang.Record` API page. However, manual inspection > and diff comparison between JDK API documentation generated before and after > this change reveals only expected differences. These differences are on the > `java.lang.R4cord` page (where the offending section is no longer displayed) > and on the pages related to the two existing records in JDK ... which are now > listed in `Record Class Hierarchy` sections in the appropriate > `package-tree.html` files. Please merge this branch with the master (they are now in conflict) and I will review the change. - PR: https://git.openjdk.java.net/jdk/pull/8523
Re: RFR: JDK-8284037: Snippet-files subdirectory not automatically detected when in unnamed package
On Fri, 13 May 2022 21:51:32 GMT, Jonathan Gibbons wrote: > Please review a small fix to address use of snippets in source code in the > unnamed package. > > The core of the fix is to replace `packageName(pkg, utils)` (which returns > `""` for the unnamed package) with plain old > `pkg.getQualifiedName().toString()` (which returns an empty string for the > unnamed package.) > > There's some minor localized cleanup and rearrangement as well. > > The test verifies behavior without and then with a source path, since that is > the likely path of discovery by a user playing with the unnamed package. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/SnippetTaglet.java line 221: > 219: var utils = writer.configuration().utils; > 220: var pkg = getPackageElement(holder, utils); > 221: var pkgLocn = utils.getLocationForPackage(pkg); Can the name be `pkgLocation`? test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetUnnamedPackage.java line 82: > 80: > 81: javadoc(args.toArray(String[]::new)); > 82: checkExit(useSourcePath ? Exit.OK : Exit.ERROR); Thanks for taking into account our discussion in #8583. On the one hand, this way of conditionally adding an option is more mouthful. On the other hand, it doesn't make a first-time reader scratch their head and makes for a cleaner test. - PR: https://git.openjdk.java.net/jdk/pull/8708
Re: RFR: JDK-8286338: suppress warnings about bad @author tags when author info is not generated.
On Thu, 12 May 2022 23:10:36 GMT, Jonathan Gibbons wrote: > > What's that `-XDdummy=dummy` you use in tests? > > It's a no-op placeholder. JavaDoc does not allow empty arguments, and the > alternative is dynamically build a list with a conditional component. `-XD` > is a "hidden" option to inject values in the compiler options map, so > `-XDdummy=dummy` just injects a dummy value there. Okay, I can see what happens when you pass an empty string: error: Illegal package name: "" I wish there were a better way of conditionally providing a javadoc option. - PR: https://git.openjdk.java.net/jdk/pull/8583
Re: RFR: JDK-8286338: suppress warnings about bad @author tags when author info is not generated.
On Fri, 6 May 2022 22:06:17 GMT, Jonathan Gibbons wrote: > Please review a simple small change to `TagletManager`, to not report > warnings about bad use of tags (such as `@author` and `@version`) when > command-line options have not been given to enable the use of the tags in the > output (i.e. the `-author` and the dreadfully-named `-version` option.) > > The change is more pragmatic than anything else. There are lots of "bad" > uses of `@author` in JDK API documentation, on methods. The alternative would > be to permit the tag anywhere, but that would be both a spec change and work > to implement the change that we're not really interest in, since the tag is > now somewhat deprecated. > > Two existing tests are updated, to test the handling of "bad" tags, with and > without the enabling option. > > Another test has some very (very) old unused lines deleted. If today was > Thursday, I'd call it Throwback Thursday. These lines predate the > `JavadocTester` conversion. Marked as reviewed by prappo (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8583
Re: RFR: JDK-8286338: suppress warnings about bad @author tags when author info is not generated.
On Fri, 6 May 2022 22:06:17 GMT, Jonathan Gibbons wrote:
> Please review a simple small change to `TagletManager`, to not report
> warnings about bad use of tags (such as `@author` and `@version`) when
> command-line options have not been given to enable the use of the tags in the
> output (i.e. the `-author` and the dreadfully-named `-version` option.)
>
> The change is more pragmatic than anything else. There are lots of "bad"
> uses of `@author` in JDK API documentation, on methods. The alternative would
> be to permit the tag anywhere, but that would be both a spec change and work
> to implement the change that we're not really interest in, since the tag is
> now somewhat deprecated.
>
> Two existing tests are updated, to test the handling of "bad" tags, with and
> without the enabling option.
>
> Another test has some very (very) old unused lines deleted. If today was
> Thursday, I'd call it Throwback Thursday. These lines predate the
> `JavadocTester` conversion.
What's that `-XDdummy=dummy` you use in tests?
test/langtools/jdk/javadoc/doclet/testSimpleTagInherit/TestSimpleTagInherit.java
line 38:
> 36: public class TestSimpleTagInherit extends JavadocTester {
> 37:
> 38: //Javadoc arguments.
This seems like an unrelated change; is it cleanup?
test/langtools/jdk/javadoc/doclet/testVersionTag/TestVersionTag.java line 57:
> 55: """
> 56: package pkg;
> 57: /** Introduction.
Cleanup?
-
PR: https://git.openjdk.java.net/jdk/pull/8583
Re: RFR: JDK-8286153: Remove redundant casts and other cleanup [v2]
On Thu, 5 May 2022 17:20:25 GMT, Jonathan Gibbons wrote: >> Please review some cleanup updates to address issues reported by an IDE. >> >> The seeds for the change were a series of redundant casts, that have now all >> been removed. Various other warnings and suggestions were made by the IDE >> for the affected files. There were a number of places with redundant type >> arguments, for which the general fix was in favor of using `var` instead of >> `<>`. >> >> Some `switch` statements were converted to the enhanced `switch` form, which >> also revealed a couple of places where `RECORD` should have been added >> alongside `ENUM`. > > Jonathan Gibbons has updated the pull request incrementally with one > additional commit since the last revision: > > address review feedback Marked as reviewed by prappo (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8543
Re: RFR: JDK-8286153: Remove redundant casts and other cleanup
On Thu, 5 May 2022 10:30:17 GMT, Pavel Rappo wrote:
>> Please review some cleanup updates to address issues reported by an IDE.
>>
>> The seeds for the change were a series of redundant casts, that have now all
>> been removed. Various other warnings and suggestions were made by the IDE
>> for the affected files. There were a number of places with redundant type
>> arguments, for which the general fix was in favor of using `var` instead of
>> `<>`.
>>
>> Some `switch` statements were converted to the enhanced `switch` form, which
>> also revealed a couple of places where `RECORD` should have been added
>> alongside `ENUM`.
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/ElementsTable.java
> line 1206:
>
>> 1204: AccessKind accessValue = null;
>> 1205: for (ElementKind kind : ALLOWED_KINDS) {
>> 1206: accessValue = switch (kind) {
>
> It feels awkward when adjacent `switch` statements use different formatting.
Correction: `switch` _expressions_ in this case.
-
PR: https://git.openjdk.java.net/jdk/pull/8543
Re: RFR: JDK-8286153: Remove redundant casts and other cleanup
On Thu, 5 May 2022 00:18:56 GMT, Jonathan Gibbons wrote:
> Please review some cleanup updates to address issues reported by an IDE.
>
> The seeds for the change were a series of redundant casts, that have now all
> been removed. Various other warnings and suggestions were made by the IDE
> for the affected files. There were a number of places with redundant type
> arguments, for which the general fix was in favor of using `var` instead of
> `<>`.
>
> Some `switch` statements were converted to the enhanced `switch` form, which
> also revealed a couple of places where `RECORD` should have been added
> alongside `ENUM`.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java
line 297:
> 295: VisibleMemberTable vmt =
> configuration.getVisibleMemberTable(enclosing);
> 296: // Check whether there is any implementation or overridden info
> to be
> 297: // printed. If not overridden or implementation info needs to be
Although awkward, it was correct before the change. Consider rewriting for
clarity or deleting it. The code is pretty self-descriptive, if you ask me.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/MetaKeywords.java
line 81:
> 79:
> results.addAll(getMemberKeywords(utils.getMethods(typeElement)));
> 80: }
> 81: results.trimToSize();
I wonder if this method will look better this way:
public List getMetaKeywords(TypeElement typeElement) {
var results = new ArrayList();
...
results.trimToSize();
return results;
}
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/MetaKeywords.java
line 125:
> 123: */
> 124: public List getOverviewMetaKeywords(String title, String
> docTitle) {
> 125: List result = new ArrayList<>(1);
Consider using `var` similarly to my suggestion above. Alternatively, we can
use two calls to `List.of(e)` and one call to `List.of()`.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/MetaKeywords.java
line 156:
> 154: }
> 155: }
> 156: results.trimToSize();
Same suggestion for `var` as above.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/ElementsTable.java line
273:
> 271: * - is specified on the command line --module
> 272: * - is derived from the module graph, that is, by expanding the
> 273: * 'requires' directive, based on --expand-requires
Thanks for using these clarifying quotes!
src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/ElementsTable.java line
1063:
> 1061: if (enclosing != null) {
> 1062: return switch (enclosing.getKind()) {
> 1063: case CLASS, ENUM, RECORD, INTERFACE,
> ANNOTATION_TYPE -> visit(enclosing);
Whoa! `RECORD` was missing. Does it make sense to accompany this PR with a
small test that crashes javadoc with a type nested in a non-included record?
src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/ElementsTable.java line
1206:
> 1204: AccessKind accessValue = null;
> 1205: for (ElementKind kind : ALLOWED_KINDS) {
> 1206: accessValue = switch (kind) {
It feels awkward when adjacent `switch` statements use different formatting.
-
PR: https://git.openjdk.java.net/jdk/pull/8543
Integrated: 8285470: Improve handling of @inheritDoc
On Fri, 22 Apr 2022 14:05:44 GMT, Pavel Rappo wrote:
> The only taglet that along with its own tag needs to know the immediately
> enclosing tag is `InheritDocTaglet`:
>
> @return {@inheritDoc}
> @throws NullPointerException {@inheritDoc}
> @param p {@inheritDoc}
>
> However, the immediately enclosing tag is unconditionally passed to all
> taglets. If we stop passing it and make `InheritDocTaglet` compute it
> instead, the code becomes cleaner.
>
> While reviewing, particularly note these benefits of the proposed change:
>
> * taglet-handling code knows less about `@inheritDoc`, and
> * `InheritDocTaglet` receives its own tag, not the tag that encloses it
This pull request has now been integrated.
Changeset: bb022b24
Author:Pavel Rappo
URL:
https://git.openjdk.java.net/jdk/commit/bb022b24cfdad9d6826140c6b26c46f480e7457d
Stats: 74 lines in 7 files changed: 5 ins; 26 del; 43 mod
8285470: Improve handling of @inheritDoc
Reviewed-by: jjg
-
PR: https://git.openjdk.java.net/jdk/pull/8361
Re: RFR: 8285470: Improve handling of @inheritDoc [v4]
On Tue, 3 May 2022 19:30:01 GMT, Pavel Rappo wrote:
>> The only taglet that along with its own tag needs to know the immediately
>> enclosing tag is `InheritDocTaglet`:
>>
>> @return {@inheritDoc}
>> @throws NullPointerException {@inheritDoc}
>> @param p {@inheritDoc}
>>
>> However, the immediately enclosing tag is unconditionally passed to all
>> taglets. If we stop passing it and make `InheritDocTaglet` compute it
>> instead, the code becomes cleaner.
>>
>> While reviewing, particularly note these benefits of the proposed change:
>>
>> * taglet-handling code knows less about `@inheritDoc`, and
>> * `InheritDocTaglet` receives its own tag, not the tag that encloses it
>
> 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 ten additional
> commits since the last revision:
>
> - Merge branch 'master' into 8285470
> - (feedback) typography
> - Update
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/InheritDocTaglet.java
>
>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
> - (fix) Fix a test failure
>
>Fixes a failure in
> jdk/javadoc/doclet/testInheritDocWithinInappropriateTag/TestInheritDocWithinInappropriateTag.java
> - (cleanup) Simplify retrieveInheritedDocumentation
> - (cleanup) Clarify retrieveInheritedDocumentation
> - (cleanup) Unify specs of commentTagsToContent
> - Stop passing "holderTag"
> - (cleanup) Remove useless null check
>
>DocFinder.search cannot return null.
Thanks, Jon.
For the reader's convenience, let me link to that tree-search support
improvement that you mentioned: JDK-8267690
(https://github.com/openjdk/jdk/pull/8369)
-
PR: https://git.openjdk.java.net/jdk/pull/8361
Re: RFR: JDK-8285869: Selective cleanup in doclint Checker class [v3]
On Tue, 3 May 2022 22:47:49 GMT, Jonathan Gibbons wrote: >> Please review some localized cleanup for the doclint Checker class, >> primarily focused on upgrading to the use of "enhanced `switch`" >> >> The output of one test was changed because of some improvements in one >> switch statement to eliminate the use of fall-through semantics. > > Jonathan Gibbons has updated the pull request incrementally with one > additional commit since the last revision: > > address review feedback Marked as reviewed by prappo (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8460
Re: RFR: JDK-8285939: javadoc java.lang.Record should not have "Direct Known Subclasses:" section
On Tue, 3 May 2022 20:38:44 GMT, Jonathan Gibbons wrote: > As described in the JBS issue, the observed problem is a side-effect of a > related but different issue, which is that record classes are not treated the > same was as enum classes when it comes to generating the class hierarchy in > `ClassTree`. Because record classes are not treated specially/differently, > they are treated as ordinary/plain classes, with the side-effect that the > page for `java.lang.Record` shows `Direct Known Subclasses`. > > The underlying fix is therefore to clone the enum support in `ClassTree` and > related classes, to provide support for record classes. It is possible to do > an extreme minimal clone, but that just clones some of the messy evolution > already there. Instead, the code in `ClassTree` and its clients is refactored > and modernized. > > Previously there were four explicit pairs of member fields, containing data > for different groups (hierarchies) of classes, namely: plain classes, enum > classes, interfaces and annotation interfaces. These fields are most of the > code to support them are moved into some new abstractions to encapsulate > related functionality. > > 1. The main new abstraction in `ClassTree` is `Hierarchy` which provides the > support for the different hierarchies displayed in the generated pages. > 2. A new enum `HierarchyKind` identifies the four existing hierarchies > (listed above) and now a new one, for record classes. The hierarchies > correspond to the different kinds of declared type. > 3. A small new class `SubtypeMap` which is a multi-map for mapping a type > element to its subtypes. This is used in `Hierarchy` and to record the > classes that implement an interfaces. > > Generally, the naming should be clearer and more obvious. The most confusing > name in the old code was `enumSubtypes` which was weird because enum classes > don't have subtypes. It meant "subtypes of supertypes of enum classes". This > was a prime motivator to switch to the `hierarchy` terminology. The variant > spellings of `intfacs` have all been replaced by `interfaces`, and > `classtree` becomes `classTree`. > > *Testing*: a new test case has been added to the `TestRecordTypes.java` test, > which verifies the new record hierarchy is displayed on a a package tree > page. It is not simple to directly test the observed/reported behavior, > because it is specific to the JDK API documentation, and because it is about > the content of the `java.lang.Record` API page. However, manual inspection > and diff comparison between JDK API documentation generated before and after > this change reveals only expected differences. These differences are on the > `java.lang.R4cord` page (where the offending section is no longer displayed) > and on the pages related to the two existing records in JDK ... which are now > listed in `Record Class Hierarchy` sections in the appropriate > `package-tree.html` files. Although I haven't looked into code in detail, I can confirm that the following JDK API pages have been updated and look good: 1. java.base/java/lang/Record.html 2. java.base/java/lang/class-use/Object.html 3. java.base/java/lang/class-use/Record.html 4. java.base/java/lang/package-use.html 5. jdk.jshell/jdk/jshell/package-tree.html 6. jdk.net/jdk/net/package-tree.html 7. overview-tree.html WRT documentation, a hierarchy of records now behave similarly to that of enums. - PR: https://git.openjdk.java.net/jdk/pull/8523
Re: RFR: JDK-8285869: Selective cleanup in doclint Checker class [v2]
On Tue, 3 May 2022 22:30:51 GMT, Jonathan Gibbons wrote: > Yes, but RECORD is handled in a different case, along with METHOD and > CONSTRUCTOR, because it can have "plain" `@param` as well as type-parameter > `@param`. In contrast, a class or interface can only have type-parameter > `@param` and never plain `@param`. You are correct; I missed that unconditionality in that second arm for`METHOD, CONSTRUCTOR, RECORD`. - PR: https://git.openjdk.java.net/jdk/pull/8460
Re: RFR: JDK-8285869: Selective cleanup in doclint Checker class [v2]
On Tue, 3 May 2022 21:53:21 GMT, Jonathan Gibbons wrote: >> Please review some localized cleanup for the doclint Checker class, >> primarily focused on upgrading to the use of "enhanced `switch`" >> >> The output of one test was changed because of some improvements in one >> switch statement to eliminate the use of fall-through semantics. > > Jonathan Gibbons has updated the pull request incrementally with one > additional commit since the last revision: > > address review feedback Marked as reviewed by prappo (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8460
Re: RFR: JDK-8285869: Selective cleanup in doclint Checker class [v2]
On Tue, 3 May 2022 21:53:21 GMT, Jonathan Gibbons wrote:
>> Please review some localized cleanup for the doclint Checker class,
>> primarily focused on upgrading to the use of "enhanced `switch`"
>>
>> The output of one test was changed because of some improvements in one
>> switch statement to eliminate the use of fall-through semantics.
>
> Jonathan Gibbons has updated the pull request incrementally with one
> additional commit since the last revision:
>
> address review feedback
I have a few more comments, Jon.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/Checker.java line
217:
> 215: hasNonWhitespaceText = false;
> 216:
> 217: implicitHeadingRank = switch (p.getLeaf().getKind()) {
(observation) Since _rank_ is a rather unusual word to see, I explored this a
bit. Numerals in H1, H2, H3, H4, H5, and H6 were somewhat implicitly referred
to as _heading levels_ by HTML4, and indeed became referred to as _ranks_ in
HTML5.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/Checker.java line
866:
> 864: if (paramElement == null) {
> 865: switch (env.currElement.getKind()) {
> 866: case CLASS, INTERFACE -> {
A record can be generic too.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/Checker.java line
1230:
> 1228: for (DocTree d: list) {
> 1229: switch (d.getKind()) {
> 1230: case TEXT -> {
Using `switch` here seems overkill.
-
PR: https://git.openjdk.java.net/jdk/pull/8460
Re: RFR: 8285470: Improve handling of @inheritDoc [v4]
> The only taglet that along with its own tag needs to know the immediately
> enclosing tag is `InheritDocTaglet`:
>
> @return {@inheritDoc}
> @throws NullPointerException {@inheritDoc}
> @param p {@inheritDoc}
>
> However, the immediately enclosing tag is unconditionally passed to all
> taglets. If we stop passing it and make `InheritDocTaglet` compute it
> instead, the code becomes cleaner.
>
> While reviewing, particularly note these benefits of the proposed change:
>
> * taglet-handling code knows less about `@inheritDoc`, and
> * `InheritDocTaglet` receives its own tag, not the tag that encloses it
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 ten additional commits since the
last revision:
- Merge branch 'master' into 8285470
- (feedback) typography
- Update
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/InheritDocTaglet.java
Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
- (fix) Fix a test failure
Fixes a failure in
jdk/javadoc/doclet/testInheritDocWithinInappropriateTag/TestInheritDocWithinInappropriateTag.java
- (cleanup) Simplify retrieveInheritedDocumentation
- (cleanup) Clarify retrieveInheritedDocumentation
- (cleanup) Unify specs of commentTagsToContent
- Stop passing "holderTag"
- (cleanup) Remove useless null check
DocFinder.search cannot return null.
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/8361/files
- new: https://git.openjdk.java.net/jdk/pull/8361/files/8bf226f7..31721e9d
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=8361=03
- incr: https://webrevs.openjdk.java.net/?repo=jdk=8361=02-03
Stats: 27873 lines in 776 files changed: 19631 ins; 3824 del; 4418 mod
Patch: https://git.openjdk.java.net/jdk/pull/8361.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/8361/head:pull/8361
PR: https://git.openjdk.java.net/jdk/pull/8361
Re: RFR: JDK-6251738: Want a top-level summary page that itemizes all spec documents referenced from javadocs (OEM spec)
On Thu, 28 Apr 2022 02:05:47 GMT, Jonathan Gibbons wrote:
> Please review the code and tests to add support for a new `@spec url title`
> tag to javadoc. Note, this does not include any changes to API doc comments
> to use the new tag in JDK API documentation; that will come later.
This is nice work. I haven't seen it in detail, but I'm plannig to do so later.
What's the logistics here? There are two interrelated bugs: 6251738 and
8226279. Are you going to add an 8226279 as an issue to this PR (`/issue add
8226279`)? What about @bug tags in tests? Are they going to mention both issues?
src/jdk.compiler/share/classes/com/sun/source/doctree/DocTreeVisitor.java line
290:
> 288: R visitSince(SinceTree node, P p);
> 289:
> 290: /**
This file uses a consistent style for doc comments for `visit` methods.
Consider adhering to that style for the time being. If you think that the style
could be improved, it should be improved in a separate PR.
src/jdk.compiler/share/classes/com/sun/source/util/DocTreeFactory.java line 340:
> 338: SnippetTree newSnippetTree(List attributes,
> TextTree text);
> 339:
> 340: /**
Similar comment to that of DocTreeVisitor: consider adhering to the file style.
src/jdk.compiler/share/classes/com/sun/source/util/DocTreeScanner.java line 512:
> 510: }
> 511:
> 512: /**
Similar comment on the file style.
src/jdk.compiler/share/classes/com/sun/source/util/DocTreeScanner.java line 532:
> 530:
> 531: /**
> 532: * {@inheritDoc} This implementation scans the children in left to
> right order.
Same as above: surprising.
src/jdk.compiler/share/classes/com/sun/source/util/DocTreeScanner.java line 544:
> 542:
> 543: /**
> 544: * {@inheritDoc} This implementation scans the children in left to
> right order.
This change is surprising; firstly, it's unrelated to this PR, secondly... why?
src/jdk.compiler/share/classes/com/sun/source/util/SimpleDocTreeVisitor.java
line 466:
> 464: }
> 465:
> 466: /**
Again: the file style.
src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DCTree.java line 1217:
> 1215: @Override
> 1216: public boolean isBlank() {
> 1217: return text.isBlank();
Can text be null?
src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DocTreeMaker.java line
419:
> 417:
> 418: @Override @DefinedBy(Api.COMPILER_TREE)
> 419: public DCSpec newSpecTree(TextTree url, List
> title) {
In DocTreeFactory the first parameter is called uri.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/ExternalSpecsWriter.java
line 68:
> 66: * Generates the file with the summary of all the references to external
> specifications.
> 67: *
> 68: * This is NOT part of any supported API.
We abolished such notes in JDK-8284362.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDoclet.java
line 33:
> 31: import java.nio.file.InvalidPathException;
> 32: import java.nio.file.Path;
> 33: import java.util.ArrayList;
This does not seem necessary for this PR and looks like an artifact of editing
in an IDE.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/SpecTaglet.java
line 44:
> 42: * A taglet that represents the {@code @spec} tag.
> 43: *
> 44: * This is NOT part of any supported API.
See my comment on such notes above.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/SpecTaglet.java
line 49:
> 47: * deletion without notice.
> 48: */
> 49: public class SpecTaglet extends BaseTaglet implements InheritableTaglet {
Ooh! This taglet is inheritable. Is it the right thing to do? If the
inheritance is not needed down the hierarchy, how can the author avoid it?
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/TagletWriter.java
line 192:
> 190: *
> 191: * @param element the element that owns the doc comment
> 192: * @param specTags the array of @spec tags.
Naked tags that aren't meant to be interpreted by javadoc make me uncomfortable.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/DocFinder.java
line 277:
> 275:
> 276: private static boolean isNonEmpty(List list) {
> 277: return list != null && !list.isEmpty();
`output.inlineTags` should never be null. Separately, can this readability
change be done in a separate PR? I do have one suitable PR in the works, which
can include it.
test/langtools/jdk/javadoc/doclet/testConditionalPages/TestConditionalPages.java
line 2:
> 1: /*
> 2: * Copyright (c) 2020, 2022, Oracle and/or its affiliates. All rights
> reserved.
Consider adding 6251738 to `@bug`.
test/langtools/jdk/javadoc/doclet/testMetadata/TestMetadata.java line 2:
> 1: /*
> 2: * Copyright (c) 2019, 2022, Oracle and/or its affiliates. All rights
> reserved.
Same as above.
test/langtools/tools/javac/diags/examples/NoTitle.java line 2:
> 1: /*
> 2: * Copyright (c)
Re: RFR: JDK-8285869: Selective cleanup in doclint Checker class
On Fri, 29 Apr 2022 00:29:50 GMT, Jonathan Gibbons wrote:
> Please review some localized cleanup for the doclint Checker class, primarily
> focused on upgrading to the use of "enhanced `switch`"
>
> The output of one test was changed because of some improvements in one switch
> statement to eliminate the use of fall-through semantics.
It's nice to see many `break` go.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/Checker.java line
737:
> 735: private Element getEnclosingPackageOrClass(Element e) {
> 736: while (e != null) {
> 737: if (e.getKind().isDeclaredType() || e.getKind() ==
> ElementKind.PACKAGE) {
This change does not seem to be equivalent: `isDeclaredType()` accepts more
kinds than the `switch` did. Does it matter here?
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/Checker.java line
866:
> 864: if (paramElement == null) {
> 865: switch (env.currElement.getKind()) {
> 866: case CLASS, ENUM, INTERFACE, ANNOTATION_TYPE -> {
Neither an enum nor annotation can be generic.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/Checker.java line
1127:
> 1125: return false;
> 1126:
> 1127: return switch (e.getKind()) {
While uniformity of constructs is good, to me, the previous variant read
better. Does this have to be a switch expression?
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/Checker.java line
1171:
> 1169:
> 1170: private boolean isDefaultConstructor() {
> 1171: return switch (env.currElement.getKind()) {
Similar to the above.
-
PR: https://git.openjdk.java.net/jdk/pull/8460
Re: RFR: 8285470: Improve handling of @inheritDoc [v3]
On Mon, 25 Apr 2022 23:12:43 GMT, Jonathan Gibbons wrote: > This one is somewhat questionable. > > Generally, the trees in `javac` and `javadoc` do not store parent pointers > (to save space), leading to a general coding philosophy of "collect any > necessary information as you walk down the tree". I understand why you want > to get rid of the extra parameters, but it is disappointing to see the code > use a tree-walk (even if we have recently improved it) to re-acquire info > that was already available a few stack-frames up. > > Since the magic/use is primarily in `TagletWriter/Impl` with most other edits > being to propagate the info, is there any better way to make the `holder` > available, without having to call `getDocTreePath`? For example, > `HtmlDocWriter` creates a `TagletWriter round about line 381: is there a way > we could stash the holder in that object for later retrieval by the > `InheritDocTaglet`. > > I would hate for this PR to be seen as a precedent for more widespread use of > `get[Doc]TreePath`. I think that this improvement in readability wins over the miniscule performance hit it brings. Currently, walking down the stack feels like telling a story with too many details too soon. Speaking of irrelevant details. The related `DocFinder` subsystem is even worse. Its `Input` and `Output` classes are full of fields used by specific taglets. I'm not a fan of this "a bag of everything" pattern. - PR: https://git.openjdk.java.net/jdk/pull/8361
Re: RFR: 8285611: Retrofit (Doc)Pretty with java.io.UncheckedIOException
On Mon, 25 Apr 2022 21:01:58 GMT, Pavel Rappo wrote: > Standard `java.io.UncheckedIOException` is more convenient than older ad-hoc > `com.sun.tools.javac.tree.(Doc)Pretty.UncheckedIOException`. Addressing your earlier points after integration. > 1. To be clear, this is more than just substitution of an equivalent > exception: there's a minor change in behavior with respect to the resulting > exception that is thrown (i.e. the original.) People may wonder, how did the > exception escape, but then again, anyone handling it ought to know. Right; however, all the important information is still on the stack. > 2. Do you want to avoid filling in the stack trace, since the unchecked > exception is transient? Maybe it's not important, since this code path is > almost never taken. I wouldn't bother; these exceptions don't warrant any future head-scratching. - PR: https://git.openjdk.java.net/jdk/pull/8387
Integrated: 8285611: Retrofit (Doc)Pretty with java.io.UncheckedIOException
On Mon, 25 Apr 2022 21:01:58 GMT, Pavel Rappo wrote: > Standard `java.io.UncheckedIOException` is more convenient than older ad-hoc > `com.sun.tools.javac.tree.(Doc)Pretty.UncheckedIOException`. This pull request has now been integrated. Changeset: e333cd33 Author: Pavel Rappo URL: https://git.openjdk.java.net/jdk/commit/e333cd33d17721bd762bfa10db1899a391556011 Stats: 20 lines in 2 files changed: 1 ins; 16 del; 3 mod 8285611: Retrofit (Doc)Pretty with java.io.UncheckedIOException Reviewed-by: jjg - PR: https://git.openjdk.java.net/jdk/pull/8387
RFR: 8285611: Retrofit (Doc)Pretty with java.io.UncheckedIOException
Standard `java.io.UncheckedIOException` is more convenient than older ad-hoc `com.sun.tools.javac.tree.(Doc)Pretty.UncheckedIOException`. - Commit messages: - Initial commit Changes: https://git.openjdk.java.net/jdk/pull/8387/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8387=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285611 Stats: 20 lines in 2 files changed: 1 ins; 16 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/8387.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8387/head:pull/8387 PR: https://git.openjdk.java.net/jdk/pull/8387
Withdrawn: 8285396: Do not fill in the stacktrace of an internal exception
On Thu, 21 Apr 2022 19:28:08 GMT, Pavel Rappo wrote: > Sometimes an exception is retrofitted into control flow to interrupt it in a > way that it wasn't originally designed to be interrupted. Such an exception > is an implementation detail. Once the exception is caught internally, it is > discarded. > > An exception has a stacktrace. Filling in a stacktrace is not free. Filling > in a stacktrace that won't be used seems like a waste. This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/8347
Re: RFR: 8285396: Do not fill in the stacktrace of an internal exception [v2]
On Thu, 21 Apr 2022 23:28:16 GMT, Pavel Rappo wrote: >> Sometimes an exception is retrofitted into control flow to interrupt it in a >> way that it wasn't originally designed to be interrupted. Such an exception >> is an implementation detail. Once the exception is caught internally, it is >> discarded. >> >> An exception has a stacktrace. Filling in a stacktrace is not free. Filling >> in a stacktrace that won't be used seems like a waste. > > Pavel Rappo has updated the pull request incrementally with one additional > commit since the last revision: > > Address feedback I'm closing this PR because we decided to address the stacktrace issue by getting rid of exceptions altogether in another PR: https://github.com/openjdk/jdk/pull/8369. However, I will file an issue to retrofit `(Doc)Pretty` with `java.io.UncheckedIOException`. - PR: https://git.openjdk.java.net/jdk/pull/8347
Re: RFR: JDK-8267690: Revisit (Doc)Tree search implemented by throwing an exception [v2]
On Mon, 25 Apr 2022 19:03:35 GMT, Jonathan Gibbons wrote: >> Please review a moderately simple cleanup change, to eliminate using an >> exception to terminate scanning a `Tree` or `DocTree` in >> `(Doc)TreePath.getPath`. >> >> The change is to set a field with the intended result, and once that field >> is set, do "best-effort" to eliminate any addition scanning. >> >> It is easy enough to stop scanning items in a list, but it is not practical >> to totally stop scanning the subsequent sibling nodes, but we can >> substantially reduce the cost of scanning those nodes, and can definitely >> avoid scanning any children of those nodes. > > Jonathan Gibbons has updated the pull request incrementally with one > additional commit since the last revision: > > improve parameter naming for internal `PathFinder` classes Marked as reviewed by prappo (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8369
Re: RFR: JDK-8267690: Revisit (Doc)Tree search implemented by throwing an exception
On Fri, 22 Apr 2022 19:10:38 GMT, Jonathan Gibbons wrote:
> Please review a moderately simple cleanup change, to eliminate using an
> exception to terminate scanning a `Tree` or `DocTree` in
> `(Doc)TreePath.getPath`.
>
> The change is to set a field with the intended result, and once that field is
> set, do "best-effort" to eliminate any addition scanning.
>
> It is easy enough to stop scanning items in a list, but it is not practical
> to totally stop scanning the subsequent sibling nodes, but we can
> substantially reduce the cost of scanning those nodes, and can definitely
> avoid scanning any children of those nodes.
I think I'm mistaken: your version does all these things I thought it wasn't
doing. What's the difference between it and Jan's version? Is it more
responsive wrt termination when the target node is found?
src/jdk.compiler/share/classes/com/sun/source/util/TreePath.java line 69:
> 67:
> 68: @Override
> 69: public TreePath scan(TreePath path, Tree docTree) {
The second parameter has nothing to do with `DocTree`. Copy-paste?
-
PR: https://git.openjdk.java.net/jdk/pull/8369
Re: RFR: JDK-8267690: Revisit (Doc)Tree search implemented by throwing an exception
On Fri, 22 Apr 2022 19:10:38 GMT, Jonathan Gibbons wrote: > Please review a moderately simple cleanup change, to eliminate using an > exception to terminate scanning a `Tree` or `DocTree` in > `(Doc)TreePath.getPath`. > > The change is to set a field with the intended result, and once that field is > set, do "best-effort" to eliminate any addition scanning. > > It is easy enough to stop scanning items in a list, but it is not practical > to totally stop scanning the subsequent sibling nodes, but we can > substantially reduce the cost of scanning those nodes, and can definitely > avoid scanning any children of those nodes. You are not changing `(Doc)TreeScanner` API, but fixing this pair of internal `(Doc)TreePathScanner` subclasses; okay. I think that your suggestion goes the extra mile as opposed to a more succinct suggestion by Jan [^1]. In particular, you avoid creating unnecessary `(Doc)TreePath` objects and calling `reduce`. Could you have a look at other cases where we use `(Doc)TreeScanner` for search? For example: * com.sun.tools.javac.tree.TreeInfo#pathFor * com.sun.tools.javac.tree.TreeInfo#containsTypeAnnotation * jdk.jshell.ExpressionToTypeInfo#findExpressionPath Maybe there's more. Is it possible to use your code there too? It would be good! [^1]: https://bugs.openjdk.java.net/browse/JDK-8267690?focusedCommentId=14448893=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14448893 - PR: https://git.openjdk.java.net/jdk/pull/8369
Re: RFR: 8285470: Improve handling of @inheritDoc [v3]
> The only taglet that along with its own tag needs to know the immediately
> enclosing tag is `InheritDocTaglet`:
>
> @return {@inheritDoc}
> @throws NullPointerException {@inheritDoc}
> @param p {@inheritDoc}
>
> However, the immediately enclosing tag is unconditionally passed to all
> taglets. If we stop passing it and make `InheritDocTaglet` compute it
> instead, the code becomes cleaner.
>
> While reviewing, particularly note these benefits of the proposed change:
>
> * taglet-handling code knows less about `@inheritDoc`, and
> * `InheritDocTaglet` receives its own tag, not the tag that encloses it
Pavel Rappo has updated the pull request incrementally with one additional
commit since the last revision:
Update
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/InheritDocTaglet.java
Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/8361/files
- new: https://git.openjdk.java.net/jdk/pull/8361/files/4c6c196b..8bf226f7
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=8361=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk=8361=01-02
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
Patch: https://git.openjdk.java.net/jdk/pull/8361.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/8361/head:pull/8361
PR: https://git.openjdk.java.net/jdk/pull/8361
Re: RFR: 8285470: Improve handling of @inheritDoc [v2]
> The only taglet that along with its own tag needs to know the immediately
> enclosing tag is `InheritDocTaglet`:
>
> @return {@inheritDoc}
> @throws NullPointerException {@inheritDoc}
> @param p {@inheritDoc}
>
> However, the immediately enclosing tag is unconditionally passed to all
> taglets. If we stop passing it and make `InheritDocTaglet` compute it
> instead, the code becomes cleaner.
>
> While reviewing, particularly note these benefits of the proposed change:
>
> * taglet-handling code knows less about `@inheritDoc`, and
> * `InheritDocTaglet` receives its own tag, not the tag that encloses it
Pavel Rappo has updated the pull request incrementally with one additional
commit since the last revision:
(fix) Fix a test failure
Fixes a failure in
jdk/javadoc/doclet/testInheritDocWithinInappropriateTag/TestInheritDocWithinInappropriateTag.java
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/8361/files
- new: https://git.openjdk.java.net/jdk/pull/8361/files/3a99bf95..4c6c196b
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=8361=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk=8361=00-01
Stats: 3 lines in 1 file changed: 0 ins; 1 del; 2 mod
Patch: https://git.openjdk.java.net/jdk/pull/8361.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/8361/head:pull/8361
PR: https://git.openjdk.java.net/jdk/pull/8361
RFR: 8285470: Improve handling of @inheritDoc
The only taglet that along with its own tag needs to know the immediately
enclosing tag is `InheritDocTaglet`:
@return {@inheritDoc}
@throws NullPointerException {@inheritDoc}
@param p {@inheritDoc}
However, the immediately enclosing tag is unconditionally passed to all
taglets. If we stop passing it and make `InheritDocTaglet` compute it instead,
the code becomes cleaner.
While reviewing, particularly note these benefits of the proposed change:
* taglet-handling code knows less about `@inheritDoc`, and
* `InheritDocTaglet` receives its own tag, not the tag that encloses it
-
Commit messages:
- (cleanup) Simplify retrieveInheritedDocumentation
- (cleanup) Clarify retrieveInheritedDocumentation
- (cleanup) Unify specs of commentTagsToContent
- Stop passing "holderTag"
- (cleanup) Remove useless null check
Changes: https://git.openjdk.java.net/jdk/pull/8361/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8361=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8285470
Stats: 74 lines in 7 files changed: 6 ins; 26 del; 42 mod
Patch: https://git.openjdk.java.net/jdk/pull/8361.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/8361/head:pull/8361
PR: https://git.openjdk.java.net/jdk/pull/8361
Re: RFR: 8285396: Do not fill in the stacktrace of an internal exception [v2]
> Sometimes an exception is retrofitted into control flow to interrupt it in a > way that it wasn't originally designed to be interrupted. Such an exception > is an implementation detail. Once the exception is caught internally, it is > discarded. > > An exception has a stacktrace. Filling in a stacktrace is not free. Filling > in a stacktrace that won't be used seems like a waste. Pavel Rappo has updated the pull request incrementally with one additional commit since the last revision: Address feedback - Changes: - all: https://git.openjdk.java.net/jdk/pull/8347/files - new: https://git.openjdk.java.net/jdk/pull/8347/files/1c69f0aa..a42a6626 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8347=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8347=00-01 Stats: 17 lines in 2 files changed: 1 ins; 16 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8347.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8347/head:pull/8347 PR: https://git.openjdk.java.net/jdk/pull/8347
Re: RFR: 8285396: Do not fill in the stacktrace of an internal exception [v2]
On Thu, 21 Apr 2022 22:10:36 GMT, Pavel Rappo wrote:
> The changes for the `UncheckedIOException` wrappers look OK, but might be
> even better if the code properly unwrapped the wrapper and simply rethrew the
> underlying cause. For example,
>
> replace
>
> ```
> } catch (UncheckedIOException ex) {
> throw new IOException(ex.getMessage(), ex);
> ```
>
> with
>
> ```
> } catch (UncheckedIOException ex) {
> throw ex.getCause();
> ```
Thanks for directing my attention to these two exceptions! I think I lost my
marbles for a good few minutes when decided to change them like that. I must've
been thinking about something else.
Those two exceptions are used as intended: for exceptional circumstances. So
their stacktraces have diagnostic value. I don't think we should re-throw the
cause, because the stacktrace might look confusing.
The only thing we could improve here is to replace ad-hoc
`UncheckedIOException` with standard `java.io.UncheckedIOException`, which
appeared much later in the codebase. Please have a look at
a42a66268f9d2175d212e6a5cba52fd11ec5332b and tell me what you think.
-
PR: https://git.openjdk.java.net/jdk/pull/8347
Re: RFR: 8285396: Do not fill in the stacktrace of an internal exception
On Thu, 21 Apr 2022 20:34:31 GMT, Jonathan Gibbons wrote:
> The changes for the `UncheckedIOException` wrappers look OK, but might be
> even better if the code properly unwrapped the wrapper and simply rethrew the
> underlying cause. For example,
>
> replace
>
> ```
> } catch (UncheckedIOException ex) {
> throw new IOException(ex.getMessage(), ex);
> ```
>
> with
>
> ```
> } catch (UncheckedIOException ex) {
> throw ex.getCause();
> ```
>
Sounds reasonable; I'll have a look at it.
> For the `TreePath` and `DocTreePath` `Result` exceptions, a better solution
> would be to add an opt-in short-circuiting mechanism info the scanner's
> `scanAndReduce` method, to avoid calling additional calls of `scan` once some
> condition has been met, like a non-null result. This would completely avoid
> the need to throw an exception.
I think that we discussed (some of) that before in the comment section of
another issue: JDK-8267690. You might want to re-read it.
What this PR proposes is to apply a band-aid until we're ready to treat the
issue properly. The treatment you are suggesting is proper, but heavyweight in
terms of the process: it involves an API change, which requires a CSR.
-
PR: https://git.openjdk.java.net/jdk/pull/8347
RFR: 8285396: Do not fill in the stacktrace of an internal exception
Sometimes an exception is retrofitted into control flow to interrupt it in a way that it wasn't originally designed to be interrupted. Such an exception is an implementation detail. Once the exception is caught internally, it is discarded. An exception has a stacktrace. Filling in a stacktrace is not free. Filling in a stacktrace that won't be used seems like a waste. - Commit messages: - Update copyright years - Initial commit Changes: https://git.openjdk.java.net/jdk/pull/8347/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8347=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285396 Stats: 15 lines in 6 files changed: 4 ins; 0 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/8347.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8347/head:pull/8347 PR: https://git.openjdk.java.net/jdk/pull/8347
Integrated: 8284908: Refine diagnostic positions for DCErroneous
On Fri, 15 Apr 2022 16:58:55 GMT, Pavel Rappo wrote:
> Before:
>
> Standard Doclet version 18+36-2087
> Building tree for all the packages and classes...
> Generating /tmp/whatever/ExampleDiagnostics.html...
> ExampleDiagnostics.java:5: error: unexpected content
> * {@docRoot a}
>^
> ExampleDiagnostics.java:11: error: unexpected content
> * {@inheritDoc a}
>^
> ExampleDiagnostics.java:17: error: '>' expected
> * @param ^
>
> After:
>
> Standard Doclet version 19-internal-2022-04-07-1105474.pavelrappo...
> Building tree for all the packages and classes...
> Generating /tmp/whatever/ExampleDiagnostics.html...
> ExampleDiagnostics.java:5: error: unexpected content
> * {@docRoot a}
> ^
> ExampleDiagnostics.java:11: error: unexpected content
> * {@inheritDoc a}
> ^
> ExampleDiagnostics.java:17: error: '>' expected
> * @param ^
This pull request has now been integrated.
Changeset: d6b5a635
Author:Pavel Rappo
URL:
https://git.openjdk.java.net/jdk/commit/d6b5a6357710598be225e67f82e6e0c1bed2d62f
Stats: 67 lines in 14 files changed: 19 ins; 0 del; 48 mod
8284908: Refine diagnostic positions for DCErroneous
Reviewed-by: jjg
-
PR: https://git.openjdk.java.net/jdk/pull/8264
Re: RFR: 8284908: Refine diagnostic positions for DCErroneous [v3]
> Before:
>
> Standard Doclet version 18+36-2087
> Building tree for all the packages and classes...
> Generating /tmp/whatever/ExampleDiagnostics.html...
> ExampleDiagnostics.java:5: error: unexpected content
> * {@docRoot a}
>^
> ExampleDiagnostics.java:11: error: unexpected content
> * {@inheritDoc a}
>^
> ExampleDiagnostics.java:17: error: '>' expected
> * @param ^
>
> After:
>
> Standard Doclet version 19-internal-2022-04-07-1105474.pavelrappo...
> Building tree for all the packages and classes...
> Generating /tmp/whatever/ExampleDiagnostics.html...
> ExampleDiagnostics.java:5: error: unexpected content
> * {@docRoot a}
> ^
> ExampleDiagnostics.java:11: error: unexpected content
> * {@inheritDoc a}
> ^
> ExampleDiagnostics.java:17: error: '>' expected
> * @param ^
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 five additional commits since
the last revision:
- Merge branch 'master' into 8284908
- Address feedback
- Update copyright years
- Specify preferred diagnostic positions
- Print prefPos in header if prefPos is defined
This is a test only change.
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/8264/files
- new: https://git.openjdk.java.net/jdk/pull/8264/files/7bce75a6..7b66b3d6
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=8264=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk=8264=01-02
Stats: 13184 lines in 918 files changed: 7976 ins; 2097 del; 3111 mod
Patch: https://git.openjdk.java.net/jdk/pull/8264.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/8264/head:pull/8264
PR: https://git.openjdk.java.net/jdk/pull/8264
Re: RFR: 8284908: Refine diagnostic positions for DCErroneous [v2]
On Wed, 20 Apr 2022 21:45:51 GMT, Jonathan Gibbons wrote: >> Pavel Rappo has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Address feedback > > test/langtools/tools/javac/doctree/DocCommentTester.java line 732: > >> 730: var n = (DCTree) node; >> 731: out.println(simpleClassName(node) + "[" + >> node.getKind() + ", pos:" + n.pos + >> 732: (n.getPreferredPosition() > n.pos ? ", >> prefPos:" + n.getPreferredPosition() : "")); > > Being paranoid, I suggest using `!=` instead of `>` If this suggestion of yours passes tests, I will integrate it. - PR: https://git.openjdk.java.net/jdk/pull/8264
Re: RFR: 8284908: Refine diagnostic positions for DCErroneous [v2]
> Before:
>
> Standard Doclet version 18+36-2087
> Building tree for all the packages and classes...
> Generating /tmp/whatever/ExampleDiagnostics.html...
> ExampleDiagnostics.java:5: error: unexpected content
> * {@docRoot a}
>^
> ExampleDiagnostics.java:11: error: unexpected content
> * {@inheritDoc a}
>^
> ExampleDiagnostics.java:17: error: '>' expected
> * @param ^
>
> After:
>
> Standard Doclet version 19-internal-2022-04-07-1105474.pavelrappo...
> Building tree for all the packages and classes...
> Generating /tmp/whatever/ExampleDiagnostics.html...
> ExampleDiagnostics.java:5: error: unexpected content
> * {@docRoot a}
> ^
> ExampleDiagnostics.java:11: error: unexpected content
> * {@inheritDoc a}
> ^
> ExampleDiagnostics.java:17: error: '>' expected
> * @param ^
Pavel Rappo has updated the pull request incrementally with one additional
commit since the last revision:
Address feedback
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/8264/files
- new: https://git.openjdk.java.net/jdk/pull/8264/files/9bcb0a22..7bce75a6
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=8264=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk=8264=00-01
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/8264.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/8264/head:pull/8264
PR: https://git.openjdk.java.net/jdk/pull/8264
Integrated: 8284697: Avoid parsing the doc comment of an element that is not documented
On Fri, 15 Apr 2022 18:23:38 GMT, Pavel Rappo wrote: > We shouldn't parse comments that are going to be thrown away. > > Although the change is simple, I have no idea on how to test it. So I'm > tempted to affix the "noreg-other" or "noreg-hard" label on the JBS issue. This pull request has now been integrated. Changeset: 13fb1eed Author:Pavel Rappo URL: https://git.openjdk.java.net/jdk/commit/13fb1eed52f1a9152242119969a9d4a0c0627513 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod 8284697: Avoid parsing the doc comment of an element that is not documented Reviewed-by: jjg - PR: https://git.openjdk.java.net/jdk/pull/8266
Re: RFR: 8284697: Avoid parsing the doc comment of an element that is not documented
On Sat, 16 Apr 2022 20:52:42 GMT, Jonathan Gibbons wrote: > The way to test it would be to put errors into a doc comment, and verify that > no errors are reported, strongly implying the comment was not parse Although we could do that, we should also keep in mind that those errors weren't reported before. If such a test is run on an unfixed version, it won't fail. This is not the behaviour one would expect from a regression test. - PR: https://git.openjdk.java.net/jdk/pull/8266
Re: RFR: 8284697: Avoid parsing the doc comment of an element that is not documented
On Fri, 15 Apr 2022 19:56:21 GMT, Pavel Rappo wrote: > > Does this need a jmh benchmark? > > I'm not sure how to compare "before" and "after" meaningfully here. Generally > speaking, I was satisfied when saw that `time` output stayed the same on my > machine; maybe only a few seconds different on a two-minute run of `make > docs`. To add to my previous reply. This PR is about correctness and genuineness, not performance: we should not touch anything unless asked to. As for performance, I'd be surprised if I learned that this fix slows javadoc down. - PR: https://git.openjdk.java.net/jdk/pull/8266
Re: RFR: 8284697: Avoid parsing the doc comment of an element that is not documented
On Fri, 15 Apr 2022 19:08:20 GMT, XenoAmess wrote: > Does this need a jmh benchmark? I'm not sure how to compare "before" and "after" meaningfully here. Generally speaking, I was satisfied when saw that `time` output stayed the same on my machine; maybe only a few seconds different on a two-minute run of `make docs`. - PR: https://git.openjdk.java.net/jdk/pull/8266
RFR: 8284697: Avoid parsing the doc comment of an element that is not documented
We shouldn't parse comments that are going to be thrown away. Although the change is simple, I have no idea on how to test it. So I'm tempted to affix the "noreg-other" or "noreg-hard" label on the JBS issue. - Commit messages: - Reorder checks Changes: https://git.openjdk.java.net/jdk/pull/8266/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8266=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284697 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8266.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8266/head:pull/8266 PR: https://git.openjdk.java.net/jdk/pull/8266
Re: RFR: 8284908: Refine diagnostic positions for DCErroneous
On Fri, 15 Apr 2022 16:58:55 GMT, Pavel Rappo wrote:
> Before:
>
> Standard Doclet version 18+36-2087
> Building tree for all the packages and classes...
> Generating /tmp/whatever/ExampleDiagnostics.html...
> ExampleDiagnostics.java:5: error: unexpected content
> * {@docRoot a}
>^
> ExampleDiagnostics.java:11: error: unexpected content
> * {@inheritDoc a}
>^
> ExampleDiagnostics.java:17: error: '>' expected
> * @param ^
>
> After:
>
> Standard Doclet version 19-internal-2022-04-07-1105474.pavelrappo...
> Building tree for all the packages and classes...
> Generating /tmp/whatever/ExampleDiagnostics.html...
> ExampleDiagnostics.java:5: error: unexpected content
> * {@docRoot a}
> ^
> ExampleDiagnostics.java:11: error: unexpected content
> * {@inheritDoc a}
> ^
> ExampleDiagnostics.java:17: error: '>' expected
> * @param ^
A drive-by observation: shouldn't we allow whitespace in between the type
variable and the closing `>`?
Currently, this is valid:
@param < T> param
while this is not:
@param param
If we decide to allow whitespace in between the type variable and the closing
`>`, it should be done in a separate PR.
-
PR: https://git.openjdk.java.net/jdk/pull/8264
RFR: 8284908: Refine diagnostic positions for DCErroneous
Before:
Standard Doclet version 18+36-2087
Building tree for all the packages and classes...
Generating /tmp/whatever/ExampleDiagnostics.html...
ExampleDiagnostics.java:5: error: unexpected content
* {@docRoot a}
^
ExampleDiagnostics.java:11: error: unexpected content
* {@inheritDoc a}
^
ExampleDiagnostics.java:17: error: '>' expected
* @param ' expected
* @param https://git.openjdk.java.net/jdk/pull/8264/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8264=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8284908
Stats: 67 lines in 14 files changed: 19 ins; 0 del; 48 mod
Patch: https://git.openjdk.java.net/jdk/pull/8264.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/8264/head:pull/8264
PR: https://git.openjdk.java.net/jdk/pull/8264
Integrated: 8283864: Clean up DocFinder and friends
On Wed, 13 Apr 2022 18:02:14 GMT, Pavel Rappo wrote: > A clean-up to facilitate more clean-up in the future. This pull request has now been integrated. Changeset: 1cc3c330 Author: Pavel Rappo URL: https://git.openjdk.java.net/jdk/commit/1cc3c330e3223944d2e20b3721ef336c87511e34 Stats: 216 lines in 11 files changed: 54 ins; 50 del; 112 mod 8283864: Clean up DocFinder and friends Reviewed-by: jjg - PR: https://git.openjdk.java.net/jdk/pull/8229
Re: RFR: 8283864: Clean up DocFinder and friends
On Wed, 13 Apr 2022 18:02:14 GMT, Pavel Rappo wrote: > A clean-up to facilitate more clean-up in the future. I'm testing the most recent commit, 7bc2fe3. If the tests pass, I'll integrate this PR. - PR: https://git.openjdk.java.net/jdk/pull/8229
Re: RFR: 8283864: Clean up DocFinder and friends [v2]
> A clean-up to facilitate more clean-up in the future. Pavel Rappo has updated the pull request incrementally with one additional commit since the last revision: Address feedback - Changes: - all: https://git.openjdk.java.net/jdk/pull/8229/files - new: https://git.openjdk.java.net/jdk/pull/8229/files/50c33364..7bc2fe39 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8229=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8229=00-01 Stats: 6 lines in 2 files changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/8229.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8229/head:pull/8229 PR: https://git.openjdk.java.net/jdk/pull/8229
Re: RFR: 8283864: Clean up DocFinder and friends
On Wed, 13 Apr 2022 23:23:53 GMT, Jonathan Gibbons wrote:
>> A clean-up to facilitate more clean-up in the future.
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/DocFinder.java
> line 136:
>
>> 134: }
>> 135:
>> 136: private Input copy(Utils utils) {
>
> Why do you need to pass in `utils` -- you can just use the value in the
> `Input` object being copied.
I don't need to pass `utils`; good catch.
-
PR: https://git.openjdk.java.net/jdk/pull/8229
Re: RFR: 8283864: Clean up DocFinder and friends
On Thu, 14 Apr 2022 11:41:59 GMT, Pavel Rappo wrote: >> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/InheritDocTaglet.java >> line 112: >> >>> 110: assert !(e instanceof TypeElement typeElement) >>> 111: || typeElement.getSuperclass().getKind() == >>> TypeKind.NONE; >>> 112: String message = utils.getSimpleName(e) + >> >> `message` sounds more freeform than the value might imply. I'd be tempted to >> call it `signature`, and replace the `""` with `e.toString()` (just in case) > > I was tempted to use `String.valueOf(e)`, but then realized that `e` is > guaranteed (although implicitly) to not be `null` by the preceding call to > `Utils.isExecutableElement`. I was tempted to use `String.valueOf(e)`, but then realized that `e` is guaranteed (although implicitly) to not be `null` by the preceding call to `Utils.isExecutableElement`. - PR: https://git.openjdk.java.net/jdk/pull/8229
