On Thu, 22 Jul 2021 11:23:13 GMT, Hannes Wallnöfer <[email protected]> wrote:
>> This change fixes a NPE in `JavadocLog` when the `DocTreePath` for an
>> inherited doc comment can't be retrieved.
>>
>> The most important change is to fix the `overriddenElement` feature in
>> `CommentHelper`. The purpose of this feature is to enable lookup of the
>> `DocTreePath` or `Element` if the comment was inherited from an overridden
>> member, but it was not implemented and used correctly. With this change, the
>> feature is implemented in `CommentHelper.getDocTreePath(DocTree)` and that
>> method is used whenever the `DocTreePath` is needed instead of duplicating
>> the functionality elsewhere.
>>
>> The `CommentHelper.setOverrideElement(Element)` method was previously used
>> in four places:
>>
>> - In `InheritDocTaglet.java` when generating a piece of inherited
>> documentation
>> - In `ReturnTaglet` when generating inherited return value documentation
>> - In `ParamTaglet` when generating inherited documentation for a parameter
>> - In `MemberSummaryBuilder` when generating inherited summary documentation
>> for an executable member
>>
>> The first three usages have been removed with this change because they were
>> not necessary. We can simply use the overridden member owning the comment
>> instead of the overriding one to generate the output. In `InheritDocTaglet`
>> we already did that, in `ReturnTaglet` and `ParamTaglet` I changed the first
>> argument to the doc-generating method from `holder` to
>> `inheritedDoc.holder`. (Note that in `ParamTaglet` the name of the parameter
>> which can change in the overriding member is passed as separate argument.)
>>
>> The remaining use of `CommentHelper.setOverrideElement(Element)` in
>> `MemberSummaryBuilder` was a bit more difficult, since the invoked method
>> `MemberSummaryWriter.addMemberSummary` generates not just the doc comment,
>> but the whole summary line including the member signature. I tried adding
>> the `CommentHelper` or `Element` owning the comment as an additional
>> parameter, but there is pretty long line of methods that must carry the
>> extra parameter around (as can be seen in the stack trace in the JBS issue).
>> In the end I decided to keep the current mechanism to register the
>> overridden holder element with the comment helper.
>>
>> One thing I am unsure about is whether it is possible for the
>> `CommentHelper` we register the overridden element on in
>> `MemberSummaryBuilder.buildSummary` to be garbage collected under extreme
>> memory pressure before it is retrieved and used again down the call graph.
>> The local reference we use to register the comment holder is no longer
>> reachable at that time and it is referenced by a `SoftReference` in
>> `CommentHelperCache`. This is probably more of a theoretical issue, and it
>> has existed before, but I thought I should mention it.
>>
>> Although it should no longer be required, I added back the null checks in
>> the methods to retrieve the `DiagnosticSource` and `DiagnosticPosition`
>> instances in `JavadocLog`. These null checks have been there in the method's
>> precursors in the `Messager` class and were dropped in JDK-8267126. As far
>> as I can tell, all the reporting functionality in `JavadocLog` accepts null
>> values for source and position, so this seemed like the right thing to do.
>>
>> Finally, as a byproduct of my attempt of adding a new parameter I dropped
>> some unused parameters in various writer classes. Since this is just cleanup
>> I can undo these changes to keep it as small as possible.
>
> Hannes Wallnöfer has updated the pull request incrementally with one
> additional commit since the last revision:
>
> JDK-8270866: Add output check for tests
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/SubWriterHolderWriter.java
line 143:
> 141: public void addSummaryLinkComment(Element member, Content
> contentTree) {
> 142: List<? extends DocTree> tags =
> utils.getFirstSentenceTrees(member);
> 143: addSummaryLinkComment(member, tags, contentTree);
I find this (a bit) surprising, since it is (potentially) useful to have access
to the enclosing writer, but that being said, if we ever really need access to
the enclosing `AbstractMemberWriter` it is probably better passed in to the
constructor.
-------------
PR: https://git.openjdk.java.net/jdk17/pull/264
