Re: 8177511: Review comments

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: 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


Reply | Threaded
Open this post in threaded view
|

Re: 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



Reply | Threaded
Open this post in threaded view
|

Re: 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




Reply | Threaded
Open this post in threaded view
|

Re: 8177511: RIP old doclet

Kumar Srinivasan
http://hg.openjdk.java.net/jdk10/jdk10/langtools/rev/ead6d6c18bcc

Kumar


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