On Wed, 8 Mar 2023 14:58:14 GMT, Pavel Rappo <[email protected]> wrote:
>> Please review this explorative refactoring for VisibleMemberTable (VMT).
>>
>> This is the first round of refactoring for VMT. This round is about *method
>> members*: declared (overriding and not) and inherited.
>>
>> During this work I gained some insight into internal workings of VMT, fixed
>> what was feasible and left TODOs and FIXMEs for everything else. Leaving
>> those comments might look untidy, but leaving them out is wasteful: they
>> clearly mark issues that should be revisited in upcoming rounds of
>> refactoring.
>>
>> As I see it today, the main issue with VMT is that implements complex and
>> error-prone computations from Java Language Specification (JLS) by hand. For
>> example, VMT interprets JLS rules for relations such as _inherits_,
>> _overrides_ and _hides_. As one would imagine, sometimes VMT does it
>> incorrectly. It would be better to eventually re-implement VMT using
>> `javax.lang.model` as much as possible. Unlike that of `jdk.javadoc`, the
>> day job of `javax.lang.model` is to provide JLS services.
>
> Pavel Rappo has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Respond to feedback
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/VisibleMemberTable.java
line 171:
> 169: * may be added from different lineages).
> 170: */
> 171: private final List<VisibleMemberTable> parents;
Should this be something different, such as a Set or a multi-map?
Generally, javac and to some extent javadoc have historically used lists for
"list-y sets".
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/VisibleMemberTable.java
line 637:
> 635: }
> 636:
> 637: private static <K, V> boolean putAllIsNonReplacing(Map<K, V> dst,
> Map<K, V> src) {
naming suggestion: the dominant initial word here is `put` but the method is
a predicate, suggesting that the initial word should be `is`. ... and/or maybe
up-level the name to be less implementation-specific (i.e. the `nonReplacing`
bit)
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/VisibleMemberTable.java
line 647:
> 645: }
> 646:
> 647: private boolean allowInheritedMethod(ExecutableElement
> inheritedMethod,
what does `allow` mean in this context? if it's inherited, doesn't that mean
it's allowed? how can an inherited method not be allowed?
-------------
PR: https://git.openjdk.org/jdk/pull/12887
