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

[Priya Lakshmi Muthuswamy]

Hi Jon,

In ModuleWriterImpl, have set the last parameter(inSummary) to the value 
true

providesTrees.put(t, commentTagsToContent(tree, mdle, 
ch.getDescription(configuration, tree), false, true));

Since this method provides contents for the module summary file, set the 
inSummary value to true.

Thanks,
Priya
On 9/20/2018 6:11 AM, Jonathan Gibbons wrote:
> 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/20180920/e7a4c735/attachment-0001.html>