Jon, This email is NOT my approval, yet. I'll have to inspect it in more detail, perhaps over the weekend. However, I may have some comments already.
1. I don't know that code base well, but I'm slowly getting there. The trick with this change is to make sure that all those configurations are interchangeable (e.g. are the same object). Now, I understand it is *likely* to be the case. What else could there be? It's just that my eyes started hurting when saw all the different ways one could get this object: utils.configuration, input.utils.configuration, writer.configuration(), through method parameters, etc. So I will focus on unraveling that ball of mud. 2. On your cleanup. You beat me to it! I had the same cleanup in the works :-) But it doesn't matter who pushes the change as long the javadoc project benefits! So my contribution would be more on the analysis side of things. What you just did is you actually fixed a bunch of bugs rather than just followed IDE advice to do away with intermediate `.stream()` calls. You see, `stream().forEach()` does NOT have to respect the order of iteration. It just happens so that it currently does. That has been letting us to get away with this for some time now. But that might change at any time. Granted, not all cases *require* that change. Sometimes we don't really care in what order a particular thing will be iterated, and may, hypothetically, benefit from some future optimization. Other times we do care. Say, you're appending something to a StringBuilder based on a series of visited nodes. Surely you want this to be done in the order those nodes naturally occur in the tree. 3. Thanks for carefully recognizing and using Collectors.joining() instead of hand-rolled loops. -Pavel > On 7 Feb 2020, at 01:17, Jonathan Gibbons <jonathan.gibb...@oracle.com> wrote: > > Please review a moderately simply change to cleanup the use of CommentHelper. > (It's a non-goal to change the functionality of CommentHelper at this time.) > > CommentHelper objects are given a reference to the BaseConfiguration when > they are constructed, but this is ignored. Subsequently, many methods that > need access to the configuration require the configuration to be passed in as > a parameter. > > By saving the value passed in to the constructor, we can remove the need for > the parameter for the various CommentHelper methods. > > The primary changes are to the signatures of the methods in CommentHelper, > removing the need for an additional configuration parameter. This affects the > use sites of those metods, but the change is just to drop the now-unnecessary > parameter. > > I also simplified the lambda expressions in CommentHelper, so that the file > is now free of IDE warnings. I stopped short of fixing names like "dtree" > and "rtree" to pass the spelling checker. > > -- Jon > > JBS: https://bugs.openjdk.java.net/browse/JDK-8238646 > Webrev: http://cr.openjdk.java.net/~jjg/8238646/webrev/ >
