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
