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

[Jonathan Gibbons]

Priya,

Sort-of OK, but can you explain more about your comment about 
ModuleWriterImpl:
> In ModuleWriterImpl, since we are writing to module summary, set the 
> inSummary parameter to true. 

I don't see any change between your two webrevs in ModuleWriterImpl, and 
so I don't
understand what that comment means.

-- Jon




On 08/29/2018 06:07 AM, Priya Lakshmi Muthuswamy wrote:
> 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/20180919/f7574a2c/attachment.html>