RFR: 8202462 : {@index} may cause duplicate labels
Jonathan Gibbons
jonathan.gibbons at oracle.com
Tue Sep 25 00:28:21 UTC 2018
Hmmm. The overload was intended to simplify the call in two places; it
hardly
seems worth it if it is only used in one place. But it can stay.
Suggest the following rewrite of the doc comment in HtmlDocletWriter.java
1309 /**
1310 * Converts inline tags and text to Content, expanding the
1311 * inline tags along the way. Called wherever text can contain
1312 * an inline tag, such as in comments or in free-form text arguments
1313 * to block tags.
1314 *
1315 * @param holderTag specific tag where comment resides
1316 * @param element specific element where comment resides
1317 * @param tags array of text tags and inline tags (often alternating)
1318 * present in the text of interest for this element
1319 * @param isFirstSentence true if text is first sentence
1320 * @param inSummary true if the comment tags are added into the summary section
1321 * @return a Content object
1322 */
Bonus points if you also do the equivalent clean up the comment in the
preceding method,
lines 1291-to 1303. :-)
No need to re-review if you just change this text.
-- Jon
On 09/20/2018 10:07 PM, Priya Lakshmi Muthuswamy wrote:
>
> Hi Jon,
>
> Yes as you mentioned, I didn't make a change between webrev.00 to
> webrev.01 in ModuleWriterImpl.java.
> The reason I mentioned it because, in the first review you had
> suggested me to use an overload for commentTagsToContent and use that
> in DocFilesHandlerImpl.java and ModuleWriterImpl.java, rather than
> making changes in these two files.
>
> So in webrev.01, I added the overload for commentTagsToContent. I
> removed the changes made in DocFilesHandlerImpl.java, but retained the
> change in ModuleWriterImpl.java.
> To explained why I retained the change, I mentioned that comment.
> /In ModuleWriterImpl, since we are writing to module summary, set the
> inSummary parameter to true. /_
>
> _I'm sorry if I was not clear in my explanation.
>
> Thanks,
> Priya
>
> On 9/21/2018 5:40 AM, Jonathan Gibbons wrote:
>> 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/20180924/fb2baae6/attachment.html>
More information about the javadoc-dev
mailing list