RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v4]

[Pavel Rappo]

On Mon, 6 Jun 2022 20:43:11 GMT, Jonathan Gibbons <jjg at openjdk.org> 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<? 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
>
> 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<ParamTree> 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