8177511: Review comments

[Jonathan Gibbons]

webrev.delta looks good.

-- Jon

On 06/21/2017 02:30 PM, Kumar Srinivasan wrote:
>
> 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/f05dccab/attachment-0001.html>