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
