8177511: Review comments

[Jonathan Gibbons]

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/20170620/6dc5855f/attachment.html>