RFR [15] 8238969: Miscellaneous cleanup

Fri Feb 14 20:03:34 UTC 2020

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

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