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

[Hannes Wallnoefer]

Thanks for the review, Jon. 

I uploaded a new webrev that includes some but not all of your suggestions. I think we should do more in a separate cleanup, especially the one to put IndexItem and SearchIndexItem in a single class hierarchy.

http://cr.openjdk.java.net/~hannesw/8237383/webrev.01/ <http://cr.openjdk.java.net/~hannesw/8237383/webrev.01/>

More details inline below.

> Am 01.04.2020 um 19:47 schrieb Jonathan Gibbons <jonathan.gibbons at oracle.com>:
> 
> 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.

The javadoc comments for both  #signature and #flatSignature were a bit oddly worded and redundant, I changed them to the following:
/**
 * Get the signature of an executable element with qualified parameter types
 * in the context of type element {@code site}.
 * For instance, for a method {@code mymethod(String x, int y)},
 * it will return {@code (java.lang.String,int)}.
 *
 * @param e the executable element
 * @param site the contextual site
 * @return String signature with qualified parameter types
 */

/**
 * Get the flat signature of an executable element with simple (unqualified)
 * parameter types in the context of type element {@code site}.
 * For instance, for a method {@code mymethod(String x, int y)},
 * it will return {@code (String, int)}.
 *
 * @param e the executable element
 * @param site the contextual site
 * @return String signature with simple (unqualified) parameter types
 */

> 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`.
> 

I added a Comparators field to HtmlDocletWriter, which removes lots of `utils.comparators` in subclasses, as well as ClassUseMapper.  In some methods that contained multiple uses I added a local variable.

> 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.

I wanted to do something along those lines in order to clean things up, but decided I had spent enough time on this already (mostly due to multiple attempts it took me to approach the issue from the right side). This is something I think we should definitely do in a separate change.

> 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?

I have changed `MemberDoc` to `element`  and `containing` to `enclosing` (the parameter name was kept from a local var in that method).


> 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.
> 

It worked because AllClassesIndexWriter only works on TypeElements, no other elements and no search tags. I have still added a null check for typeElement:

if (typeElement == null || !utils.isCoreClass(typeElement)) {
    continue;
}

> 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.
> 

I tried to fix this but didn’t find a solution to the circular dependency I was really happy with, so I left the comparators in Utils.

> 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.
> 

Also didn’t try to improve this.

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

Since all the @param and @return tags in that class end with a period and it was not really part of the fix I removed that change from the patch.

> 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`.

I was aware of this, but found a few other instances of classes in `toolkit` importing and using code from `formats.html`, so I thought it was permissible :) I left this unchanged for now and would propose to fix it with a future IndexItem/SearchIndexItem cleanup.

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

Good question, I haven’t tried, will look into it. 

> 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)

Actually this code looked suspicious to me. I just added the condition to check if the type is an annotation before adding a field as annotation type field, as is done for methods and required/optional annotation members. Since this is not part of the fix I might as well remove this change. I have left it in for now.

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

Fixed.

> -- Jon
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/javadoc-dev/attachments/20200403/0570b0b8/attachment.htm>