RFR: 8237383: Members inherited from non-public types are not included in index

[Jonathan Gibbons]

Hi Hannes,

I don't see anything wrong in the webrev, so in that sense, the webrev 
is Approved.  I did have one question, about VisibleMemberTable, but I 
see you anticipated my question and answered it below, in the original 
RFR email. :-)

The work itself looks good. Nice.

The remaining comments are stylistic or suggestions.

AbstractExecutableMemberWriter: it took a while to realize why you were 
adding a typeElement parameter to utils.flatSignature. Maybe some better 
comments on Utils.flatSignature would help.

Utils.comparators. There's a lot of uses of `utils.comparators`. When it 
appears more than once, I would suggest stashing a copy of comparators 
somewhere, so that uses of `utils.comparators` simplify to just 
`comparators`.

IndexItem ... did you consider making IndexItem an interface or abstract 
class, with subtypes for the two different kinds ... introducing 
ElementIndexItem, and possibly seeing if SearchIndexItem becomes a 
possible subtype of IndexItem. We could then (eventually) use the 
upcoming switch on sealed types in upcoming releases.

AbstractIndexWriter:268 ... MemberDoc is old-speak for "Element", and 
could be updated. Equally, it might be interesting to (separately) grep 
for instances of such old-speak, and fix them.

AbstractIndexWriter:269: parameter `containing`: there is a standard 
word in the Language Model jargon: `enclosing`.  Do you mean `enclosing` 
or something different?

AllClassesIndexWriter:137.  I guess this works because utils.isCoreClass 
is null-tolerant ... but I did have to check that. maybe it would be 
better to defensively check if `typeElement != null`... or if you move 
towards subtypes of IndexItem, check the class of the subtype here.  Or 
have an `is...` method on IndexItem.

BaseConfiguration/Utils/Comparators.
In general, {Base,Html}Configuration is the "god object" from which to 
access handlers/managers/utilities.  I realize there is a 
chicken-and-egg problem setting up Comparators (since there is a 
circular dependency with Utils), but once you're past that, then it 
would be better if BaseConfiguration is the place to get Comparators. To 
that end, I would make the field in Utils private, to discourage direct 
use of the field from outside the class.

Comparators:
In general, we have way too many `get` methods that ought to be `new`, 
`make` or `build` methods. Comparators has the opposite problem ... it 
has `make` methods that return shared instances.  I would either rename 
the `make*` methods to be `get*`, or even drop the verb altogether and 
have methods like `comparators.generalPurposeComparator()`. The recent 
changes to option handling used verb-less accessors.  If all the 
accessor names end in `Comparator` maybe even that word could be dropped.

TagletWriter:192
By convention, the phrase used to describe a paramter in `@param` should 
not end with a period `.`

IndexBuilder:36
It is an anti-pattern for a class in the `toolkit` world to import a 
class from the `formats.html` world.  I don't see anything in 
`SearchIndexItem` that is specific to HTML, so SearchIndexItem 
could/should be moved to the `toolkit` package, alongside `IndexItem`.

IndexBuilder:56
(Performance) would it be better to build the indexMap unsorted, and 
then just sort it when needed? (Open question.)

VisibleMemberTable:630...644
It seems questionable add members into two separate lists. To me, it 
would seem more intuitive to add the member into one group -or- the 
other.  This may come up as an issue if we get to merge the handling of 
anno types to the general handling for other kinds of types (enum, 
class, interface, record, etc)

Comparators: (multiple) missing blank line before documentation comment.

-- Jon

On 3/27/20 5:03 PM, Hannes Wallnöfer wrote:
> Please review:
>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8237383
> Webrev: http://cr.openjdk.java.net/~hannesw/8237383/webrev.00/
>
> This patch makes sure members inherited from non-documented parents are included in HTML and search index files.
>
> There is a new IndexItem class to the doclets.toolkit.util package that is used instead of raw Elements to collect indexed elements by IndexBuilder. This is necessary because we need to know the inheriting class when rendering a member signature as the member type may change by instantiating type parameters.
>
> IndexItem can also hold a SearchIndexItem so that index tags can be added to IndexBuilder to generate index files from a single collection rather than manually merging two collections.
>
> Unfortunately I ran into some obstacles with this, so the resulting code is not quite as simple and unified as I would have liked. HTML and search indexes are structured differently and each use a variety of different formats and comparators for sorting. This makes it very difficult to manage everything within a single place, so there’s still much of the original redundancy and complexity left.
>
> I also moved all comparator factory methods from the Utils class to a new Comparators class. The code is mostly unchanged as I was not able to simplify it without changing behaviour. There is one new comparator factory method that generates a composite generator for the IndexItems containing elements and search tags in the HTML index.
>
> The changes in VisibleMemberTree are mostly removal of dead code. The implementation of getExtraMembers(Kind) was the same as getAllVisibleMembers(Kind), so no additional members were found and added by that method. There is also a bit of code cleanup in that class.
>
>
> Thanks,
> Hannes