<html>

  <head>

    <meta http-equiv="Content-Type" content="text/html; charset=utf-8">

  </head>

  <body text="#000000" bgcolor="#FFFFFF">

    <h2>phase-1</h2>

    <p><b>In AbstractIndexWriter</b>, <br>

      the use of "anyOf" with a singleton list seems a bit weird.</p>

    <p><b>In SearchIndexItem</b>:<br>

      The word "index" is heavily overloaded in javadoc. Would it be

      better in this case to use "INDEX_TAG"? (Just asking).<br>

      If nothing else, add doc-comments on the members of Category.</p>

    <p>Folding "boolean systemProperty" into the Category seems like a

      good idea.<br>

    </p>

    <p><b>In SearchIndexItems</b>:Â Â  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.</p>

    <pre><span class="changed"> 120      * <p> The returned stream consists of all items {@code i} for which there's</span></pre>

    <p>It's often considered better to avoid contractions (like

      "there's") in formal writing.</p>

    <pre><span class="changed"> 120      * <p> The returned stream consists of all items {@code i} for which there's</span>

<span class="changed"> 121      * a category {@code c}, from the specified categories, such that</span>

<span class="changed"> 122      * {@code i.getCategory().equals(c)}. The stream may be empty, if there are</span>

<span class="changed"> 123      * no such items.</span></pre>

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

    <p>`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!Â </p>

    <p><br>

    </p>

    <h2>phase-2</h2>

    <p>AbstractIndexWriter<br>

      line 511,512: horrible non-standard formatting: was this suggested

      by an IDE?<br>

    </p>

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

      Be careful of using Character.toUpperCase/toLowerCase ... they are

      locale-dependent.<br>

      That being said, maybe we do want the locale-dependent version

      here, which is unusual.<br>

      While it is reasonable to get the first character of a Java

      identifier, I wonder if it is reasonable<br>

      to get the first character of an arbitrary string, i.e. from an

      {@index} tag.</p>

    <p>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</p>

    <p><br>

    </p>

    <h2>phase-3</h2>

    <p>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!)<br>

    </p>

    <p>SystemPropertiesWriter:80<br>

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

    <p>SystemPropertiesWriter:155<br>

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

    </p>

    <p>TagletWriterImpl:457<br>

      The comment is unclear: does "those" refer back to

      DocFileELement?Â  I'm also curious why you think using identity

      equals and hashCode is inefficient.</p>

    <p>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.)</p>

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

    <p><br>

    </p>

    <h2>phase-4</h2>

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

    <p>Line 172: I see the comment moved here from TagletWriterImpl.

      Same comments apply that I made for phase-3.<br>

    </p>

    <p>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!<br>

    </p>

    <p><br>

    </p>

    <p> <br>

    </p>

    <p> </p>

  </body>

</html>