RFR [15] 8238969: Miscellaneous cleanup

Fri Feb 14 21:57:45 UTC 2020

Responding to the details in your message.  Comments inline.

On 02/13/2020 07:50 AM, Pavel Rappo wrote:
> 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.

Generally OK

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

I think it is a mis-feature of the API design that Iterable.forEach and 
Stream.forEach
sounds so similar yet have such an important difference in semantics. 
This makes
me want to avoid Stream.forEach entirely. I'm OK with generally keeping code
deterministic and restricting use to Iterable.forEach.

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

Hmmm. it was not clear to me that " ".repeat(4) was a better alternative 
to "    ".
At 18, yes, it's beginning to be more useful.

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

As I noted in the review, I don't think these comments belong in the code.
They are interesting work product, however.
That being said, there are (at least) 3 subsets here.
1. composing localizable lists in strings, e.g. for error messages
2. composing non-localizable lists in strings (thinking of the search 
index .js files)
3. composing localizable lists in Content nodes

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

Probably the result of checking that Serializable items have 
serialVersionUID at some point.

>
> 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()?

Various aspects here:

  * It is surprising/disappointing that .equals is defined for one impl
    of Name and not the other, or more accurately, that it is defined
    differently for both.
  * It is surprising/disappointing that there is no fast-track for `==`
    in the one case where it is implemented.
  * While it is true that javac Name is defined/used such that `==` is
    sufficient, there are not such explicit claims on the
    javax.lang.model super-interface that was retrofitted in JDK 6. So,
    unless we change javax.lang.model.Name, it is probably reasonable to
    make .equals faster and change to using that.  But (separately) I
    note that javadoc (the tool in particular) is fundamentally
    dependent on javac and its internals, and so it is not all bad that
    we make assumptions on that basis.

If we follow up, it should be done separately.

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

I'd want to see the list and proceed cautiously, and separately, perhaps 
even on a
class-by-class basis.

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/javadoc-dev/attachments/20200214/f4492192/attachment-0001.htm>