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

Tue Apr 7 16:33:59 UTC 2020

Looks good to me.

As well as the primary fix, I like the big step forwards cleaning up 
comparators, and the simplification/removal of VisibleMemberTable "extra 
members".

-- Jon

On 4/3/20 9:55 AM, Hannes Wallnoefer wrote:
> 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/
>
> More details inline below.
>
>> Am 01.04.2020 um 19:47 schrieb Jonathan Gibbons 
>> <jonathan.gibbons at oracle.com <mailto: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/20200407/0246fcec/attachment-0001.htm>