On Sun, 5 Jun 2022 20:55:12 GMT, Pavel Rappo <pra...@openjdk.org> 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 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<? extends XTree>` are read-only
> * An argument of the `List<XTree>` type can still be passed to a `List<?
> extends XTree>` 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
Finished reviewing May 25 commits.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java
line 70:
> 68: */
> 69: private static Map<String, String> mapNameToPosition(Utils utils,
> List<? extends Element> params) {
> 70: Map<String, String> result = new HashMap<>();
Is there a reason not to change this to `Map<String, Integer>`
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));
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java
line 108:
> 106: ExecutableElement md = (ExecutableElement) input.element;
> 107: CommentHelper ch = utils.getCommentHelper(md);
> 108: List<? extends ParamTree> tags = input.isTypeVariableParamTag
This sort of cleanup is nice, and welcome.
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.
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
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`
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java
line 194:
> 192: * Returns a {@code Content} representation of a list of {@code
> ParamTree}.
> 193: *
> 194: * Warns about {@code @param} tags that do not map to parameter
> elements
It does more than just warn ...
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java
line 209:
> 207: List<? extends Element>
> formalParameters,
> 208: TagletWriter writer) {
> 209: Map<String, ParamTree> tagOfPosition = new HashMap<>();
I still think that one of the type parameters should be `Integer`
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java
line 281:
> 279: * @param name the 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 :-(
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"
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`?
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ThrowsTaglet.java
line 112:
> 110: Content result = writer.getOutputInstance();
> 111: Set<String> alreadyDocumented = new HashSet<>();
> 112: result.add(throwsTagsOutput(tagsMap, writer, alreadyDocumented,
> typeSubstitutions, true));
I presume `throwsTagsOutput` does not include the header...?
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ThrowsTaglet.java
line 115:
> 113: result.add(inheritThrowsDocumentation(holder, thrownTypes,
> alreadyDocumented, typeSubstitutions, writer));
> 114: result.add(linkToUndocumentedDeclaredExceptions(thrownTypes,
> alreadyDocumented, writer));
> 115: return result;
Content.add returns `this`, so you could simplify this further to
return writer.getOutletInstance()
.add(throwsTagOutput....)
.add(inheritThrowsDocumentation...)
.add(linktoUndocumentedExceptions...);
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ThrowsTaglet.java
line 140:
> 138: TypeMirror t2 = i2.next();
> 139: if (!types.isSameType(t1, t2))
> 140: map.put(t1.toString(), t2);
lots of opportunities for `var` here!
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
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.
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?
-------------
PR: https://git.openjdk.java.net/jdk/pull/8886
