RFR [15] 8238969: Miscellaneous cleanup

[Pavel Rappo]

Hello,

Please review the change for https://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