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
