Jon,
I have updated the webrev: http://cr.openjdk.java.net/~prappo/8238969/webrev.01/ The main changes are: 1. Removed the "Consider this while working on JDK-8238966" comments. I have also updated the JDK-8238966 issue, adding a link to this thread so the discussion and the findings won't be lost. 2. Downgraded uses of Stream.forEach to Collections' forEach. It is both correct and less noisy. Come to think of it, there's one particular case where Collections' forEach is definitely more readable than that of Stream's. It's Map.forEach. Here you get a benefit of the key-value pair being already bound to separate variables, rather than to Map.Entry. Which is nice because you can give them meaningful names. 3. As for nanoTime, it's a hiccup from me working on core-libs networking. Even though in this particular case time is NOT used to control stuff (e.g. timeout), nanoTime is still a correct way of measuring the elapsed time. Being monotonic, it is immune to many problems in this area. So, there's value in that change as well. All the feedback you've provided so far has been considered. Most of the changes have been included in that new webrev. All the tests pass in our CI system. -Pavel > On 14 Feb 2020, at 21:57, Jonathan Gibbons <jonathan.gibb...@oracle.com> > wrote: > > 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 >> >> >> > >
