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
>