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