On Thu, 22 Jul 2021 11:23:13 GMT, Hannes Wallnöfer <hann...@openjdk.org> 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 Thanks for the review, Jon! I did a recursive diff on the generated docs before and after this change, there are no differences. I'm leaving the `JavadocLog` checks out as you suggest. ------------- PR: https://git.openjdk.java.net/jdk17/pull/264
