RFR [15] 8238969: Miscellaneous cleanup

Fri Feb 14 20:17:30 UTC 2020

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

-- Jon

On 02/14/2020 12:03 PM, Jonathan Gibbons wrote:
>
> 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 forhttps://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/6577d672/attachment-0001.htm>