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
