Re: RFR [15] 8238969: Miscellaneous cleanup

[Jonathan Gibbons]

Maybe avoid streams and .forEachOrdered in Utils:510 by simply rewriting 
the line

    modifiers.forEach(m -> append(m.toString());
-- Jon


On 02/14/2020 12:17 PM, Jonathan Gibbons wrote:

Ooops, my mistake. I misunderstodd the difference between .forEach and 
.forEachOrdered.

-- Jon


On 02/14/2020 12:03 PM, Jonathan Gibbons wrote:

Pavel,

Mostly good; thanks for looking at this. There's one significant 
error which needs to be addressed.  I also think the comments '// 
Consider this...' should be removed. The information may be 
interesting, and at least is preserved in this webrev + patch file, 
but the comments don't belong in the main source code.

----

I don't care for some of the whitespace changes, but I won't object 
to any reformatting that is the result of reformatting in an IDE like 
IntelliJ with default settings.

I don't like the notes to the future of the form: "// Consider this 
while ...".  I think such notes belong outside the codebase, in some 
other form.  I don't know of any precedent elsewhere in the 
(langtools) code base.

AbstractMemberWriter ... it looks like it's only used one. I would 
inline its contents to the use site and remove the method. It doesn't 
carry its own weight, and the preferred replacement is easy enough to 
use directly.  javadoc has too many methods which are just shorter 
rewritings of otherwise acceptable calls.

HtmlDocletWriter ... the constructor comment was wrong, but the new 
comment is not very helpful. Suggest a standard comment of the form 
"Creates an {@code HtmlDocletWriter}."  If you file me an RFE, I will 
fix the bad doc comment and do other cleanup for this file.  For 
example, the constructor should be protected, since this class is 
always intended to be a supertype.

ModuleWriterImpl:  you've (uncharacteristically) change .forEach to 
.stream().forEach.  Why?

ModuleWriterImpl:  in a number of places, you've renamed local 
variables, such as exportPkgList -> exportedPackagesSet. Consider 
dropping the collection-kind from the name, as in, "exportPkgList -> 
exportedPackages".  Consider renaming lambda parameter name 
'directive" to just "d", especially when the scope is small.

TagletManager: I'm not sure about the new comment at 373. I don't 
think it is javadoc's job to detect mis-spellings, but "wrong case" 
may just be common enough to be worth checking for, especially when 
the correct name is mixed-case, like {@docRoot}, {@systemProperty}

Utils.addModifier(line 510): forEachOrdered in WRONG. Modifiers 
should not be alphabetically sorted by name! A better change would be 
to change the parameter type to SortedSet, and then rely on the 
sort-order of the argument set.

Utils: in a couple of places, you have used 'new ArrayList<DocTree>'. 
Did you try diamond syntax?  I'm sort-of surprised if you can't use 
it there.

Utils.Pair: it's on _my_ list to move this class out of Utils, but 
the changes here are OK for now.

Start: the change to use nanoTime seems gratuitous. If you're going 
to edit this code, I suggest renaming "tm" to "start" or "startTime", 
and change line 552 to declare and use "elapsedTime" instead of 
reusing (and changing the units of) "tm".

-- Jon

On 02/13/2020 07:50 AM, Pavel Rappo wrote:
Hello,

Please review the change forhttps://bugs.openjdk.java.net/browse/JDK-8238969:

  http://cr.openjdk.java.net/~prappo/8238969/webrev.00/

During the last exploratory debugging session, I collected a number of issues
that I think are worth fixing. These include:

1. Indentation, unnecessary (and hopefully non-controversial) parentheses,
local variables' names, typos, method references (where practical), comments, 
etc.

2. Usages of the java.util.stream API. This could be thought of as complementary
change to that [1] of by Jonathan Gibbons, though it stemmed from an unrelated
investigation of mine.

In a nutshell, some sites that use streams are order-sensitive, while others
are not. Where the order is important, I used forEachOrdered() or changed the
thing straight to Iterable.forEach(), eliding the in-between call to stream().
In some cases that allowed to simplify the thing into an atomic (as in 
"indivisible",
not "concurrent-atomic") operation from Collections API. For example:

     - jdk/javadoc/internal/doclets/formats/html/LinkFactoryImpl.java:136,138
     - jdk/javadoc/internal/doclets/toolkit/util/Utils.java:3268

Where the order was immaterial, I emphasized that by ensuring a 
non-deterministic
semantics was used. For that reason, in some places I inserted an in-between 
call
to stream(), if there was none previously.

     I'm still undecided on whether to use streams where the order is 
immaterial.
     It's not about optimization, but readability. I guess it all depends on the
     context and potential for using intermediate operations. It seems to be a
     no-brainer when no intermediate operations, such as filtering or mapping,
     are involved.

Sadly, we don't seem to have good tools at hand for testing things like that.
One way this change could be tested is through using "unfriendly" (work-to-rule)
implementations of Collections and Stream APIs. For example, if the order of
iteration is unspecified, or better still, explicitly specified to be 
non-deterministic,
the implementation should make sure that the order is random. If a List is not
of the RandomAccess flavor it should impose a perceivable penalty for getting
elements by index, a call to Thread.sleep or a busy-loop would do.

I'm sure such things exist in one form or another. We just don't have them, and
implementing those on our own is a project not for the faint-hearted! Immutable
collections, introduced in JEP 269, are good in this sense, though they are not
enough:

     The iteration order of set elements/mappings is unspecified and is subject
     to change

But I digress. The existing battery of tests passes.

3. Pinpoint uses of modern String APIs (and boy, did they fit in nicely!)

    - jdk/javadoc/internal/doclets/formats/html/AbstractMemberWriter.java:202
    - jdk/javadoc/internal/tool/Start.java:289,291

4. Marking with in-line comments "Consider this while working on JDK-8238966"

This is about future work on extracting joining-with-a-separator functionality.
I believe those comments have value, even though the issue they mark is not
addressed in this patch. Some of the sites seem to be a low-hanging fruit, but
the point is to consider them as a whole before proceeding with a fix.

5. Removal of the Result.serialVersionUID field

I couldn't think of any use for that field. My best guess is that it's just a
leftover from the times when this enum was Error.

Notes
=====

a. jdk/javadoc/internal/doclets/formats/html/AbstractTreeWriter.java:143

Shouldn't it use equals() instead of `==` in this case? A quick look shows a
surprising number of reference equality checks on javax.lang.model.element.Name
and javax.lang.model.element.Element instances. Why would we need to use
reference equality on types with explicitly defined equals() and hashCode()?

b. What do people think of removing unused type members?

Since jdk.javadoc doesn't seem to be using java.lang.reflect (beyond a couple of
trivial interactions with Doclet SPI), the compile-time checking should be 
enough
to make sure the removal is safe.

The are some 100 (hundred) of unused methods across the jdk.javadoc codebase.

-Pavel

-------------------------------------------------------------------------------
[1]https://mail.openjdk.java.net/pipermail/javadoc-dev/2020-February/001390.html