RFR: 8280713: Related to comment inheritance jdk.javadoc cleanup and refactoring [v3]

[Jonathan Gibbons]

On Fri, 25 Feb 2022 17:46:05 GMT, Pavel Rappo <prappo at openjdk.org> wrote:

>> Explorative refactoring performed while looking into multiple `@inheritDoc` issues. The easiest way to review it is to, probably, go commit by commit; they are quite focused and commented. Not only the branch as a whole, but all the constituent commits should pass tests and leave JDK API Documentation unchanged.
>
> Pavel Rappo has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Fix outdated code
>    
>    Uses a set instead of list for quick method search. In a future commit we'll try to figure out why `found` are not unique.
>  - Fix outdated code
>    
>    Fix outdated inline comments and names. Both the assertion and the `contains` method will be removed in a future commit.

Commit: Remove ImplementedMethods::removeOverriddenMethod

I'm not convinced by this one. I'd like to see more investigation.   I read your description and follow that, but I still think there may be an issue of what the code does, vs. what it was intended to do in a complex inheritance hierarchy.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/VisibleMemberTable.java line 1012:

> 1010:             for (TypeMirror interfaceType : intfacs) {
> 1011:                 // TODO: this method also finds static methods which are pseudo-inherited;
> 1012:                 //  this needs to be fixed

I can't add this comment where I'd like to ... line 992, the declaration of the `ImplementedMethods` class.
Especially given the "curious" nature of the constructor, it would help to have descriptive comments on either the class or constructor.  It seems like `ImplementedMethods` should be a collection of methods implemented by a `TypeElement` (similar to filtering `Element.getEnclosedElements`. But is seems like this collection is more like the collection of methods in a class hierarchy that :"match" a given signature .. perhaps too get a candidate list of methods rom which to gather documentation (e.g with `{@inheritDoc}`).

Given that, would it be a reasonable alternative rationale for removing the `removeOverriddenMethod` because by construction, nothing ever gets added that subsequently needs to be removed?

I'm still trying to understand this one. I think I know what it is trying to do, but I'm concerned that while it might be trying to do something useful, it might be implemented incorrectly ... in which case, it should be fixed, not removed.

I guess I'd be interest to hear the actual/intended operation when a collection of different possibly-unrelated interfaces all implement the "same" method according to its signature.   Think: the "graphical cowboy" issue.   Two unrelated interfaces that both implement the `draw()` method. Now create a hierarchy of subtypes of those interfaces that start overriding methods with either simple overrides on not-simple overrides.  Then see if `removeOverriddenMethod` kicks in. My guess is that this situation is uncommon and does not arise in JDK API or even our tests.

-------------

PR: https://git.openjdk.java.net/jdk/pull/7233