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

[Jonathan Gibbons]

Priya,

You have expanded more on what your comment means, but you've not 
explained why
you are telling me this.

In particular, note the following two aspects of your revised review request

1. You wrote:
> 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. 

2. But you did not modify ModuleWriterImpl in webrev.01 compared to 
webrev.00.
In particular, this horribly long command shows you did not make any 
additional changes to ModuleWriterImpl.java in webrev.01
$ diff 
cr.openjdk.java.net/~pmuthuswamy/8202462/webrev.0{0,1}/raw_files/new/src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/ModuleWriterImpl.java


So, since you have made a point of telling me about that change, I'm 
saying that I don't see that change in the updates you made to webrev.01.
Conversely, if you didn't mean to make any additional change to 
ModuleWriterImpl.java, then why did you go out of your way to mention 
the change?

-- Jon




On 09/19/2018 08:42 PM, Priya Lakshmi Muthuswamy wrote:
>
> 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/af2e4d96/attachment.html>