On Wed, 21 Jul 2021 16:59:20 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. This pull request has now been integrated. Changeset: fbe28e4e Author: Hannes Wallnöfer <hann...@openjdk.org> URL: https://git.openjdk.java.net/jdk17/commit/fbe28e4ee1f1ff7fb617c2e1f96c04f4b371fa2b Stats: 136 lines in 8 files changed: 76 ins; 46 del; 14 mod 8270866: NPE in DocTreePath.getTreePath() Reviewed-by: jjg ------------- PR: https://git.openjdk.java.net/jdk17/pull/264
