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

[Priya Lakshmi Muthuswamy]

Hi Jon,

Thanks for the comment.

updated webrev: http://cr.openjdk.java.net/~pmuthuswamy/8202462/webrev.01/
In ModuleWriterImpl, since we are writing to module summary, set the 
inSummary parameter to true.

Regards,
Priya

On 8/29/2018 5:42 AM, Jonathan Gibbons wrote:
> 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/20180829/0cb7e827/attachment.html>