phase-1
*In AbstractIndexWriter*,
the use of "anyOf" with a singleton list seems a bit weird.
*In SearchIndexItem*:
The word "index" is heavily overloaded in javadoc. Would it be better in
this case to use "INDEX_TAG"? (Just asking).
If nothing else, add doc-comments on the members of Category.
Folding "boolean systemProperty" into the Category seems like a good idea.
*In SearchIndexItems*: ugh for having to put the apiNote after the
scary comment. Nothing we can do, except maybe one day think about
removing the scary comments, which have maybe outlived their usefulness.
Maybe we could limit them to package-info.java fies.
120 * <p> The returned stream consists of all items {@code i} for which
there's
It's often considered better to avoid contractions (like "there's") in
formal writing.
120 * <p> The returned stream consists of all items {@code i} for which
there's
121 * a category {@code c}, from the specified categories, such that
122 * {@code i.getCategory().equals(c)}. The stream may be empty, if
there are
123 * no such items.
The commas are questionable around "from the specified categories".
Don't use commas when the enclosed text is important to the
understanding of the sentence.
`concat` seems overkill. For an internal method, I'd simplify the
anyOfCategories, and either use overloads or a single varargs and just
verify the length is not zero. By inspection, it seems to only ever use
1 or 2 args. Use overloads!
phase-2
AbstractIndexWriter
line 511,512: horrible non-standard formatting: was this suggested by an
IDE?
keyCharacter: when is this method called with an empty string? It looks
like it is related to the SearchIndexItem having en empty label ...
perhaps we should check/fix the problem there.
Be careful of using Character.toUpperCase/toLowerCase ... they are
locale-dependent.
That being said, maybe we do want the locale-dependent version here,
which is unusual.
While it is reasonable to get the first character of a Java identifier,
I wonder if it is reasonable
to get the first character of an arbitrary string, i.e. from an {@index}
tag.
various: it's not wrong, but I personally don't like the style of
importing static methods, like groupingBy and toList. Keep it if you
want, but if I were writing the code, I would use Collectors.methodName
phase-3
HtmlDocletWriter: this class is one instance per page ... do you really
mean to put it here, as compared to Configuration (more global) or some
more specific kind of page? (I see it moved in phase 4!)
SystemPropertiesWriter:80
I don't know if this is the right or wrong place for the check, but for
reference I would compare with other "optional" pages, like Constant
Values and Serialized Form.
SystemPropertiesWriter:155
I like the intention, but I would have thought the link factory would
have returned a link in code font. I guess I would check other places
where links are generated.
TagletWriterImpl:457
The comment is unclear: does "those" refer back to DocFileELement? I'm
also curious why you think using identity equals and hashCode is
inefficient.
Utils: uuugh ... I keep trying to get stuff *out* of the Utils dumping
ground, and here you are adding to it. This is OK for now, but there may
be a different utility class waiting to be written for processing
DocTrees. This is not the only method to be working on DocTrees
(although other work may be elsewhere, down in jdk.compiler module.)
test: the new convention is to generate files on the fly ... which will
be even easier when we have text blocks. The ToolBox library class has
code to help write out smal files like these ... and using text strings
avoids the heavy copyright overhead.
phase-4
In SystemPropertiesWriter, there is no need to use a WeakHashMap
compared to a standard HashMap. Elements created by javac, which is the
dominant collection of elements, will be permanent until the end of the
javadoc run. And moreover, SystemPropertiesWriter should have a short
lifetime, and no elements will be garbage-collectible while it is alive.
Line 172: I see the comment moved here from TagletWriterImpl. Same
comments apply that I made for phase-3.
In SearchIndexItem, if I understand it correctly, it seems strange to
add a field of type DocletElement that is specific to certain types of
search index item. And, there is a cognizant disparity between the name
of the type (DocletElement) and the name of the method (getElement).
Generally, I would expect a method named `getElement` to return an
Element. It seems like all other serach-index items are associated with
n element anyway!
