RFR: 8157000: Do not generate javadoc for overridden method with no spec change

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

RFR: 8157000: Do not generate javadoc for overridden method with no spec change

Kumar Srinivasan
Hello,

Please review for 8157000, please let me know if you have any
comments or ways I can improve it.

The "before and after" test without the flag is identical.

http://cr.openjdk.java.net/~ksrini/8157000/webrev.00/index.html

Thanks
Kumar
Bug: https://bugs.openjdk.java.net/browse/JDK-8157000
CSR: https://bugs.openjdk.java.net/browse/JDK-8187386
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8157000: Do not generate javadoc for overridden method with no spec change

Jonathan Gibbons
HtmlConfiguration.java
Other public fields nearby have javadoc comments. It would be nice to
follow the precedent.
That being said, see comments for Utils.java:1568. This option would be
better defined in
BaseConfiguration, not HtmlConfiguration.

MethodWriterImpl.java 293
No need to assign null. Better to leave it empty (just "Content label;")
so that the compiler
will verify it is asigned correctly in the statements that follow.

MethodWriterImpl.java 295,296
I acknowledge you've followed the style of 300,301, but as a comment for
general cleanup
we should move away from legacy wrapper methods like
configuration.getText in favor
if the more explicit underlying call resources.getText. Or we could do
that as part of a more
comprehensive cleanup.

resources/standard.properties:371
<...> is normally used for "fill in your own value here", as in <name>,
<url>, etc
Use (...|...) to indicate alternatives. See line 389 for an example.
         (all|none|[-]<group>)

MemberSumaryBuilder.java:485
Would be good to find a semantically helpful name instead of just
inhmembers0.

MemberSumaryBuilder.java:486,487,490,517
The short names are sort-of OK, but "members" should be capitalized.
Also "class" in "inhclass"

MemberSumaryBuilder.java:497, 517
"footnote" is one word, not two. Also implies   addSummaryFootnote

Utils.java:1566
Since this is for methods only, it would be better if the parameter type
was
ExecutableElement, not Element. That should be OK with the callsite at
MemberSummaryBuilder:498
Also, the name is not ideal. How about isSimpleOverride?

Utils.java:1568
The need to cast to HtmlConfiguration is a big indication that something
is defined
in the wrong place. summarizeOveriddenMethods is not HTML-specific and
so should
be moved to BaseConfiguration.

VisibleMemberMap.java:241,316
Is the cast safe? You're iterating the members, but assuming the member
is an
ExecutableElement

Test files:
I assume I'm seeing false/duplicate output from the recent repo
consolidation.
The first group of 3 test files should not be here.

TestOverrideMethods.java
a) if you're going to have long strings in checkOutput, I suggest blank
lines to help separate the
individual strings.
b) Long strings will be annoying to maintain going forward. Ideally, you
should reduce the strings
to the minimum necessary to focus on what you need to test, so that
unrelated work on nearby
content doesn't break these tests.

-- Jon


On 09/21/2017 02:10 PM, Kumar Srinivasan wrote:

> Hello,
>
> Please review for 8157000, please let me know if you have any
> comments or ways I can improve it.
>
> The "before and after" test without the flag is identical.
>
> http://cr.openjdk.java.net/~ksrini/8157000/webrev.00/index.html
>
> Thanks
> Kumar
> Bug: https://bugs.openjdk.java.net/browse/JDK-8157000
> CSR: https://bugs.openjdk.java.net/browse/JDK-8187386

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8157000: Do not generate javadoc for overridden method with no spec change

Kumar Srinivasan
Hi Jon,

Thanks for the review, here is a new webrev:
http://cr.openjdk.java.net/~ksrini/8157000/webrev.01/index.html
and a delta webrev:
http://cr.openjdk.java.net/~ksrini/8157000/webrev.01/webrev.delta/

> HtmlConfiguration.java
> Other public fields nearby have javadoc comments. It would be nice to
> follow the precedent.
> That being said, see comments for Utils.java:1568. This option would
> be better defined in
> BaseConfiguration, not HtmlConfiguration.

Done

>
> MethodWriterImpl.java 293
> No need to assign null. Better to leave it empty (just "Content
> label;") so that the compiler
> will verify it is asigned correctly in the statements that follow.
>
Done
> MethodWriterImpl.java 295,296
> I acknowledge you've followed the style of 300,301, but as a comment
> for general cleanup
> we should move away from legacy wrapper methods like
> configuration.getText in favor
> if the more explicit underlying call resources.getText. Or we could do
> that as part of a more
> comprehensive cleanup.

Agreed. Will look into filing a bug.

>
> resources/standard.properties:371
> <...> is normally used for "fill in your own value here", as in
> <name>, <url>, etc
> Use (...|...) to indicate alternatives. See line 389 for an example.
>         (all|none|[-]<group>)

Done

>
> MemberSumaryBuilder.java:485
> Would be good to find a semantically helpful name instead of just
> inhmembers0.

Done,  renamed.

>
> MemberSumaryBuilder.java:486,487,490,517
> The short names are sort-of OK, but "members" should be capitalized.
> Also "class" in "inhclass"

Done, renamed.

>
> MemberSumaryBuilder.java:497, 517
> "footnote" is one word, not two. Also implies   addSummaryFootnote

Fixed

>
> Utils.java:1566
> Since this is for methods only, it would be better if the parameter
> type was
> ExecutableElement, not Element. That should be OK with the callsite at
> MemberSummaryBuilder:498
> Also, the name is not ideal. How about isSimpleOverride?

Done.

>
> Utils.java:1568
> The need to cast to HtmlConfiguration is a big indication that
> something is defined
> in the wrong place. summarizeOveriddenMethods is not HTML-specific and
> so should
> be moved to BaseConfiguration.

Done.

>
> VisibleMemberMap.java:241,316
> Is the cast safe? You're iterating the members, but assuming the
> member is an
> ExecutableElement

Yes the cast  is safe!, what happens is the VMM is being built for each
*kind* so in
the method getClassMembers, the member kinds are obtained on the
VMM kind.

>
> Test files:
> I assume I'm seeing false/duplicate output from the recent repo
> consolidation.
> The first group of 3 test files should not be here.
>

Oops,  removed those extraneous files.


> TestOverrideMethods.java
> a) if you're going to have long strings in checkOutput, I suggest
> blank lines to help separate the
> individual strings.
> b) Long strings will be annoying to maintain going forward. Ideally,
> you should reduce the strings
> to the minimum necessary to focus on what you need to test, so that
> unrelated work on nearby
> content doesn't break these tests.

Ok I narrowed down the focus, and eliminated the long lines.

Thanks

Kumar

>
> -- Jon
>
>
> On 09/21/2017 02:10 PM, Kumar Srinivasan wrote:
>> Hello,
>>
>> Please review for 8157000, please let me know if you have any
>> comments or ways I can improve it.
>>
>> The "before and after" test without the flag is identical.
>>
>> http://cr.openjdk.java.net/~ksrini/8157000/webrev.00/index.html
>>
>> Thanks
>> Kumar
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8157000
>> CSR: https://bugs.openjdk.java.net/browse/JDK-8187386
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8157000: Do not generate javadoc for overridden method with no spec change

Jonathan Gibbons
Looks good to me.
Thanks for the delta-webrev.

-- Jon

On 10/03/2017 02:47 PM, Kumar Srinivasan wrote:

> Hi Jon,
>
> Thanks for the review, here is a new webrev:
> http://cr.openjdk.java.net/~ksrini/8157000/webrev.01/index.html
> and a delta webrev:
> http://cr.openjdk.java.net/~ksrini/8157000/webrev.01/webrev.delta/
>
>> HtmlConfiguration.java
>> Other public fields nearby have javadoc comments. It would be nice to
>> follow the precedent.
>> That being said, see comments for Utils.java:1568. This option would
>> be better defined in
>> BaseConfiguration, not HtmlConfiguration.
>
> Done
>
>>
>> MethodWriterImpl.java 293
>> No need to assign null. Better to leave it empty (just "Content
>> label;") so that the compiler
>> will verify it is asigned correctly in the statements that follow.
>>
> Done
>> MethodWriterImpl.java 295,296
>> I acknowledge you've followed the style of 300,301, but as a comment
>> for general cleanup
>> we should move away from legacy wrapper methods like
>> configuration.getText in favor
>> if the more explicit underlying call resources.getText. Or we could
>> do that as part of a more
>> comprehensive cleanup.
>
> Agreed. Will look into filing a bug.
>
>>
>> resources/standard.properties:371
>> <...> is normally used for "fill in your own value here", as in
>> <name>, <url>, etc
>> Use (...|...) to indicate alternatives. See line 389 for an example.
>>         (all|none|[-]<group>)
>
> Done
>
>>
>> MemberSumaryBuilder.java:485
>> Would be good to find a semantically helpful name instead of just
>> inhmembers0.
>
> Done,  renamed.
>
>>
>> MemberSumaryBuilder.java:486,487,490,517
>> The short names are sort-of OK, but "members" should be capitalized.
>> Also "class" in "inhclass"
>
> Done, renamed.
>
>>
>> MemberSumaryBuilder.java:497, 517
>> "footnote" is one word, not two. Also implies addSummaryFootnote
>
> Fixed
>
>>
>> Utils.java:1566
>> Since this is for methods only, it would be better if the parameter
>> type was
>> ExecutableElement, not Element. That should be OK with the callsite
>> at MemberSummaryBuilder:498
>> Also, the name is not ideal. How about isSimpleOverride?
>
> Done.
>
>>
>> Utils.java:1568
>> The need to cast to HtmlConfiguration is a big indication that
>> something is defined
>> in the wrong place. summarizeOveriddenMethods is not HTML-specific
>> and so should
>> be moved to BaseConfiguration.
>
> Done.
>
>>
>> VisibleMemberMap.java:241,316
>> Is the cast safe? You're iterating the members, but assuming the
>> member is an
>> ExecutableElement
>
> Yes the cast  is safe!, what happens is the VMM is being built for
> each *kind* so in
> the method getClassMembers, the member kinds are obtained on the
> VMM kind.
>
>>
>> Test files:
>> I assume I'm seeing false/duplicate output from the recent repo
>> consolidation.
>> The first group of 3 test files should not be here.
>>
>
> Oops,  removed those extraneous files.
>
>
>> TestOverrideMethods.java
>> a) if you're going to have long strings in checkOutput, I suggest
>> blank lines to help separate the
>> individual strings.
>> b) Long strings will be annoying to maintain going forward. Ideally,
>> you should reduce the strings
>> to the minimum necessary to focus on what you need to test, so that
>> unrelated work on nearby
>> content doesn't break these tests.
>
> Ok I narrowed down the focus, and eliminated the long lines.
>
> Thanks
>
> Kumar
>
>>
>> -- Jon
>>
>>
>> On 09/21/2017 02:10 PM, Kumar Srinivasan wrote:
>>> Hello,
>>>
>>> Please review for 8157000, please let me know if you have any
>>> comments or ways I can improve it.
>>>
>>> The "before and after" test without the flag is identical.
>>>
>>> http://cr.openjdk.java.net/~ksrini/8157000/webrev.00/index.html
>>>
>>> Thanks
>>> Kumar
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8157000
>>> CSR: https://bugs.openjdk.java.net/browse/JDK-8187386
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8157000: Do not generate javadoc for overridden method with no spec change

Kumar Srinivasan

Hi Jon,

Inspections of the specdiff pointed out some missing links, in the
previous revision.

In this iteration:
1. All missing links have been purged.
2. Fixing the above exposed issues relating to:
     a. handling of javafx properties
    b. missing ids in class-use and index
    c.  latent bug in the initialization of enum methods (value/valueof)
comments
    d. made some adjustments to VisibleMemberMap and else where.
    e. add some more test use-cases.

My qparent:
47320[qparent]:47318,47319   8b09673f7ede   2017-10-06 20:54 +0000   lana


Full webrev:
http://cr.openjdk.java.net/~ksrini/8157000/webrev.02/

The delta webrev is here:
http://cr.openjdk.java.net/~ksrini/8157000/webrev.02/webrev.delta/

Specdiff
http://cr.openjdk.java.net/~ksrini/8157000/specdiff.out/overview-summary.html

API docs before:
http://cr.openjdk.java.net/~ksrini/8157000/docs-before/api/overview-summary.html

API docs after:
http://cr.openjdk.java.net/~ksrini/8157000/docs-after/api/overview-summary.html

Thanks
Kumar



> Looks good to me.
> Thanks for the delta-webrev.
>
> -- Jon
>
> On 10/03/2017 02:47 PM, Kumar Srinivasan wrote:
>> Hi Jon,
>>
>> Thanks for the review, here is a new webrev:
>> http://cr.openjdk.java.net/~ksrini/8157000/webrev.01/index.html
>> and a delta webrev:
>> http://cr.openjdk.java.net/~ksrini/8157000/webrev.01/webrev.delta/
>>
>>> HtmlConfiguration.java
>>> Other public fields nearby have javadoc comments. It would be nice
>>> to follow the precedent.
>>> That being said, see comments for Utils.java:1568. This option would
>>> be better defined in
>>> BaseConfiguration, not HtmlConfiguration.
>>
>> Done
>>
>>>
>>> MethodWriterImpl.java 293
>>> No need to assign null. Better to leave it empty (just "Content
>>> label;") so that the compiler
>>> will verify it is asigned correctly in the statements that follow.
>>>
>> Done
>>> MethodWriterImpl.java 295,296
>>> I acknowledge you've followed the style of 300,301, but as a comment
>>> for general cleanup
>>> we should move away from legacy wrapper methods like
>>> configuration.getText in favor
>>> if the more explicit underlying call resources.getText. Or we could
>>> do that as part of a more
>>> comprehensive cleanup.
>>
>> Agreed. Will look into filing a bug.
>>
>>>
>>> resources/standard.properties:371
>>> <...> is normally used for "fill in your own value here", as in
>>> <name>, <url>, etc
>>> Use (...|...) to indicate alternatives. See line 389 for an example.
>>>         (all|none|[-]<group>)
>>
>> Done
>>
>>>
>>> MemberSumaryBuilder.java:485
>>> Would be good to find a semantically helpful name instead of just
>>> inhmembers0.
>>
>> Done,  renamed.
>>
>>>
>>> MemberSumaryBuilder.java:486,487,490,517
>>> The short names are sort-of OK, but "members" should be capitalized.
>>> Also "class" in "inhclass"
>>
>> Done, renamed.
>>
>>>
>>> MemberSumaryBuilder.java:497, 517
>>> "footnote" is one word, not two. Also implies addSummaryFootnote
>>
>> Fixed
>>
>>>
>>> Utils.java:1566
>>> Since this is for methods only, it would be better if the parameter
>>> type was
>>> ExecutableElement, not Element. That should be OK with the callsite
>>> at MemberSummaryBuilder:498
>>> Also, the name is not ideal. How about isSimpleOverride?
>>
>> Done.
>>
>>>
>>> Utils.java:1568
>>> The need to cast to HtmlConfiguration is a big indication that
>>> something is defined
>>> in the wrong place. summarizeOveriddenMethods is not HTML-specific
>>> and so should
>>> be moved to BaseConfiguration.
>>
>> Done.
>>
>>>
>>> VisibleMemberMap.java:241,316
>>> Is the cast safe? You're iterating the members, but assuming the
>>> member is an
>>> ExecutableElement
>>
>> Yes the cast  is safe!, what happens is the VMM is being built for
>> each *kind* so in
>> the method getClassMembers, the member kinds are obtained on the
>> VMM kind.
>>
>>>
>>> Test files:
>>> I assume I'm seeing false/duplicate output from the recent repo
>>> consolidation.
>>> The first group of 3 test files should not be here.
>>>
>>
>> Oops,  removed those extraneous files.
>>
>>
>>> TestOverrideMethods.java
>>> a) if you're going to have long strings in checkOutput, I suggest
>>> blank lines to help separate the
>>> individual strings.
>>> b) Long strings will be annoying to maintain going forward. Ideally,
>>> you should reduce the strings
>>> to the minimum necessary to focus on what you need to test, so that
>>> unrelated work on nearby
>>> content doesn't break these tests.
>>
>> Ok I narrowed down the focus, and eliminated the long lines.
>>
>> Thanks
>>
>> Kumar
>>
>>>
>>> -- Jon
>>>
>>>
>>> On 09/21/2017 02:10 PM, Kumar Srinivasan wrote:
>>>> Hello,
>>>>
>>>> Please review for 8157000, please let me know if you have any
>>>> comments or ways I can improve it.
>>>>
>>>> The "before and after" test without the flag is identical.
>>>>
>>>> http://cr.openjdk.java.net/~ksrini/8157000/webrev.00/index.html
>>>>
>>>> Thanks
>>>> Kumar
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8157000
>>>> CSR: https://bugs.openjdk.java.net/browse/JDK-8187386
>>>
>>
>