8177511: Review comments

[Kumar Srinivasan]

Hi Jon,

I noticed while I did a personal review there was one test being ignored
test/tools/javadoc/MaxWarns.java, provided a test doclet to simulate the
behavior of the html doclet, thatthe test expects.

Besides the above I made some minor cosmetic changes.
Shown here in the delta webrev:
http://cr.openjdk.java.net/~ksrini/8177511/webrev-03/webrev/webrev.delta/

Please use this patch for comparison purposes:
http://cr.openjdk.java.net/~ksrini/8177511/webrev-03/a8e4d2286eba.patch

The mercurial tip is at: a8e4d2286eba

Thanks
Kumar


> OK,
>
> typo in a test .. "implentations"
>
> suggest clean up comment in Start re: old tool and old doclet's Start
>
> suggest for future: deprecate old entry point for removal.
>
> (no need for another big webrev.)
>
> -- Jon
>
>
> On 06/19/2017 01:51 PM, Kumar Srinivasan wrote:
>>
>> Ok here are the new changes, also please see my inlined responses below.
>>
>> http://cr.openjdk.java.net/~ksrini/8177511/webrev-02/
>>
>> Full webrev
>> http://cr.openjdk.java.net/~ksrini/8177511/webrev-02/webrev
>>
>> Delta webrev
>> http://cr.openjdk.java.net/~ksrini/8177511/webrev-02/webrev.delta/
>>
>> Please use this patch directly generated by hg, the patch generated by
>> webrev wrt. the directories containing removed binaries especially 
>> images.
>> http://cr.openjdk.java.net/~ksrini/8177511/webrev-02/8177511.patch
>>
>>> (Completed this round of review.)
>>>
>>> -- Jon
>>>
>>> These review comments are based on a meld comparison, derived from 
>>> the webrev langtools.patch.
>>>
>>> # Comparing src directories
>>>
>>> /w/jjg/work/reviews/jdk10.remove.doclet/langtools/src/jdk.javadoc/share/classes/com/sun/tools/doclets/standard/Standard.java 
>>>
>>> line 42, remove debug print.
>> Fixed
>>>
>>> Suggest leaving in
>>>
>>>
>>> /**
>>>  * This is not the doclet you are looking for.
>>>  * @deprecated The doclet has been superseded by its replacement,
>>>  * {@code jdk.javadoc.doclets.StandardDoclet}.
>>>  */
>>> @Deprecated(forRemoval=true, since="9")
>>
>> Fixed
>>>
>>>
>>> There is stuff left in 
>>> com.sun.tools.doclets.internal.toolkit.resources that has not been 
>>> deleted.  Please ensure that all these files are gone. Most of the 
>>> cruft seems to be image files.  Try doing
>>>     hg remove --after 
>>> src/jdk.javadoc/share/classes/com/sun/tools/doclets/internal
>> No it is patch generated by webrev please see explanation above
>>>
>>> /w/jjg/work/reviews/jdk10.remove.doclet/langtools.ref/src/jdk.javadoc/share/classes/com/sun/tools/doclets/standard/package-info.java 
>>>
>>> line 28, suggest replacing "superseded" by "replaced".
>>
>> Fixed
>>>
>>> /w/jjg/work/reviews/jdk10.remove.doclet/langtools.ref/src/jdk.javadoc/share/classes/com/sun/tools/javadoc/main/DocImpl.java 
>>>
>>> change from FatalError to Error looks suspicious.  That being said, 
>>> I don't see any code to catch/handle FatalError so maybe this is OK.
>>
>> yes should be ok.
>>
>>>
>>> Start.java
>>> Why has hasOldTaglet not gone away?
>>> Why has OldStdDocletName not gone away?
>>> Why has "Step 5" (lines 769-773) not gone away
>>
>> Fixed, also eradicated -Xold.
>>>
>>> # Comparing test directories
>>>
>>> I see some undeleted jar files.  Is that just a webrev/meld artifact 
>>> like the images in src/ ?
>>
>> Webrev/hg I think, please use the above patch.
>>
>>>
>>> /w/jjg/work/reviews/jdk10.remove.doclet/langtools.ref/test/tools/javadoc/6942366/T6942366.java 
>>>
>>> If you add @ignore, it should be followed by a bug number and synopsis.
>>> Alternative is to provide/use a new toy doclet that reports included 
>>> classes, or else just delete the test.
>> Fixed it, I think the test can be improved perhaps in the new doclet.
>>
>>>
>>> /w/jjg/work/reviews/jdk10.remove.doclet/langtools.ref/test/tools/javadoc/6964914/Test.java 
>>>
>>> Not sure why you've had to edit this.
>>
>> Ah, note the test is invoking the std doclet with doclint, but 
>> doclint is invoked by the doclet
>> so removed the error and option which is no longer recognized by the 
>> *silly* doclet.
>>>
>>> /w/jjg/work/reviews/jdk10.remove.doclet/langtools.ref/test/tools/javadoc/badSuper/BadSuper.java 
>>>
>>> Delete the test
>>
>> Done.
>>
>>>
>>> /w/jjg/work/reviews/jdk10.remove.doclet/langtools/test/tools/javadoc/lib/ToyDoclet.java 
>>>
>>> Would it be useful to have in the new world as well? (Separate RFE, 
>>> I guess)
>>
>> Yep
>>
>>>
>>> /w/jjg/work/reviews/jdk10.remove.doclet/langtools/test/tools/javadoc/parser/7091528/T7091528.java 
>>>
>>> hmmm, OK; it wasn't a very good test to begin with, and now it's 
>>> even less good. But OK.
>>
>> Fixed it
>>
>>
>> Thanks
>> Kumar
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/javadoc-dev/attachments/20170621/21936e89/attachment.html>