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

-- Jon

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


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:

Please review the change for

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, 

2. Usages of the 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 
not "concurrent-atomic") operation from Collections API. For example:

     - jdk/javadoc/internal/doclets/formats/html/,138
     - jdk/javadoc/internal/doclets/toolkit/util/

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

     I'm still undecided on whether to use streams where the order is 
     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 
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

     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/
    - jdk/javadoc/internal/tool/,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.


a. jdk/javadoc/internal/doclets/formats/html/

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 
to make sure the removal is safe.

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



Reply via email to