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