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.gibb...@oracle.com <mailto:jonathan.gibb...@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
