On Tue, 17 May 2022 00:18:07 GMT, Jonathan Gibbons <[email protected]> wrote:
>> As described in the JBS issue, the observed problem is a side-effect of a
>> related but different issue, which is that record classes are not treated
>> the same was as enum classes when it comes to generating the class hierarchy
>> in `ClassTree`. Because record classes are not treated
>> specially/differently, they are treated as ordinary/plain classes, with the
>> side-effect that the page for `java.lang.Record` shows `Direct Known
>> Subclasses`.
>>
>> The underlying fix is therefore to clone the enum support in `ClassTree` and
>> related classes, to provide support for record classes. It is possible to do
>> an extreme minimal clone, but that just clones some of the messy evolution
>> already there. Instead, the code in `ClassTree` and its clients is
>> refactored and modernized.
>>
>> Previously there were four explicit pairs of member fields, containing data
>> for different groups (hierarchies) of classes, namely: plain classes, enum
>> classes, interfaces and annotation interfaces. These fields are most of the
>> code to support them are moved into some new abstractions to encapsulate
>> related functionality.
>>
>> 1. The main new abstraction in `ClassTree` is `Hierarchy` which provides the
>> support for the different hierarchies displayed in the generated pages.
>> 2. A new enum `HierarchyKind` identifies the four existing hierarchies
>> (listed above) and now a new one, for record classes. The hierarchies
>> correspond to the different kinds of declared type.
>> 3. A small new class `SubtypeMap` which is a multi-map for mapping a type
>> element to its subtypes. This is used in `Hierarchy` and to record the
>> classes that implement an interfaces.
>>
>> Generally, the naming should be clearer and more obvious. The most confusing
>> name in the old code was `enumSubtypes` which was weird because enum classes
>> don't have subtypes. It meant "subtypes of supertypes of enum classes".
>> This was a prime motivator to switch to the `hierarchy` terminology. The
>> variant spellings of `intfacs` have all been replaced by `interfaces`, and
>> `classtree` becomes `classTree`.
>>
>> *Testing*: a new test case has been added to the `TestRecordTypes.java`
>> test, which verifies the new record hierarchy is displayed on a a package
>> tree page. It is not simple to directly test the observed/reported
>> behavior, because it is specific to the JDK API documentation, and because
>> it is about the content of the `java.lang.Record` API page. However, manual
>> inspection and diff comparison between JDK API documentation generated
>> before and after this change reveals only expected differences. These
>> differences are on the `java.lang.R4cord` page (where the offending section
>> is no longer displayed) and on the pages related to the two existing records
>> in JDK ... which are now listed in `Record Class Hierarchy` sections in the
>> appropriate `package-tree.html` files.
>
> Jonathan Gibbons has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains three commits:
>
> - merge with upstream master
> - fix copyright; update test description
> - JDK-8285939: javadoc java.lang.Record should not have "Direct Known
> Subclasses:" section
This is a nice cleanup! My only objection is that some refactored/moved methods
have lost their doc comment.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AbstractTreeWriter.java
line 94:
> 92: }
> 93:
> 94: protected void addTree(Hierarchy hierarchy, String heading, Content
> content) {
You should keep and adapt the doc comment of the addTree method you removed
above.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/ClassTree.java
line 100:
> 98: }
> 99:
> 100: public SortedSet<TypeElement> allSubtypes(TypeElement
> typeElement) {
Again I think it would be nice to preserve (and update) the doc comment of the
method. I also think the naming of `subtypes` and `allSubtypes` is not super
clear, so maybe the comments should explicitly mention returning direct or
"recursive" subclasses.
-------------
Marked as reviewed by hannesw (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8523
