RFR [15] 8238969: Miscellaneous cleanup

Fri Feb 14 21:13:07 UTC 2020

Maybe avoid streams and .forEachOrdered in Utils:510 by simply rewriting 
the line

     modifiers.forEach(m -> append(m.toString());

-- Jon


On 02/14/2020 12:17 PM, Jonathan Gibbons wrote:
>
> 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/f85ca33c/attachment.htm>