RFR: 8202462 : {@index} may cause duplicate labels

[Jonathan Gibbons]

DocFilesHandlerImpl.java

OK. It's a bit of a shame to have to edit this file. In 
HtmlDOcletWriter, you added an overload for getTagletWriterInstance, but 
not for commentTagsToContent.

HtmlDocletWriter.java

366: If you're going to add new javadoc comments, do it 
properly/correctly; don't just cut-n-paste something that was incomplete 
to begin with!

All the calls to addCommentTags are hard to read, with the growing 
number of boolean parameters. This is OK for now, but maybe we should 
used enums instead of booleans in some places to aid readability.

1221 you've added a parameter but not updated the doc comment. See next 
comment re: naming.

1233 summarySection ... suggest changing the name to be more like a 
predicate, such as "inSummary" or "isSummary"

ModuleWriterImpl.java

Same comment as for DocFilesHandlerImpl.java

TagletWriter.java

Instead of having a non-final field, you're better to have the first 
constructor call the second, like this:

   74     private final boolean summarySection;
   75
   76     public TagletWriterImpl(HtmlDocletWriter htmlWriter, boolean isFirstSentence) {
              this(htmlWriter, isFirstSentence, false);
   81     }
   82
   83     public TagletWriterImpl(HtmlDocletWriter htmlWriter, boolean isFirstSentence, boolean summarySection) {
   77         super(isFirstSentence);
   78         this.htmlWriter = htmlWriter;
   79         configuration = htmlWriter.configuration;
   80         this.utils = configuration.utils;
   85         this.summarySection = summarySection;
   86     }

various:  in this file you're unnecessarily qualifying member references 
with "this.", as in code like this:

	if (this.isFirstSentence && this.summarySection)

That's not a coding style for javadoc or even JDK. Please remove 
unnecessary use of "this."

See previous comments about naming of "summarySection".

TestModules.java

OK

TestIndexTaglet.java

29: I'm mildly surprised that jtreg does not complain about the "@" in 
"@index", but OK if it doesn't

Overall, it's a pretty weak test. This is especially so when you look at 
lines 73-75.

   73         checkOrder("pkg/A.html",
   74                 "<h3>Method Summary</h3>\n",
   75                 "a");

What that is saying is, make sure that after the "Method Summary" 
heading, the character "a" appears. What are the chances of a false 
positive there? :-)  The indexed term needs to be much more unique, such 
as "xyzzy" (look it up) or "Crumpets" or something.  And then in line 
75, expand the check to include what the word *should* be surrounded 
with.   As written, the test would incorrectly succeed if the doclet 
incorrectly was not changed, because all you are checking for is the 
basic text, not the text in context.

-- Jon


On 08/14/2018 01:58 AM, Priya Lakshmi Muthuswamy wrote:
> Hi,
>
> Kindly review the fix for 
> https://bugs.openjdk.java.net/browse/JDK-8202462
> webrev : http://cr.openjdk.java.net/~pmuthuswamy/8202462/webrev.00/
>
> Thanks,
> Priya

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/javadoc-dev/attachments/20180828/80f906a6/attachment.html>