code review request for 6880112, Coin: use diamond in core libraries

Joe Darcy joe.darcy at oracle.com
Fri Dec 17 03:18:39 UTC 2010


On 12/16/2010 7:14 PM, Stuart Marks wrote:
> Thanks for the comments, folks.
>
> I've been puzzling over the issue of joining lines after diamond 
> conversion if they're short enough. It turns out to be a surprisingly 
> complex interaction of tools, code style, and process.[1]
>
> Joe and I actually talked about this issue briefly the other day and 
> our initial inclination was not to make the additional whitespace 
> changes. In some sense the Jackpot automated converter should do this 
> for us, but it doesn't, and I don't want to take the time at the 
> moment to get up to speed and modify it to do this.
>
> However, Brian and Alan and another person off-list have all raised 
> this issue, so I think it's important that we address it. Looking at 
> it again, leaving something like this in the code:
>
>     private static Map<String, URL> urls
>         = new HashMap<>(10);
>
> really offends my sensibilities, and let's face it, nobody is going to 
> go in and fix this unless I do it now. It turns out that it isn't too 
> difficult to write a script that can be run after-the-fact to join 
> these lines if they end up shorter than 80 characters.
>
> Unfortunately, as Kelly points out (and I've also verified this) 
> changing the line breaks will change the .class file contents (though 
> not the bytecodes). The original changeset does have a very nice 
> property of not changing the .class files at all. We could preserve 
> this property by putting the line joins in a separate changeset. I'd 
> do this if we could have multiple changesets with the same bugid, but 
> a separate changeset requires a separate bugid... so maybe this isn't 
> worth the overhead after all.
>
> I've run the line joiner and it changed ten files. Here's an 
> incremental webrev, over and above yesterday's:
>
>     http://cr.openjdk.java.net/~smarks/reviews/6880112/webrev.1/
>
> and the corresponding hg bundle is here:
>
>     http://cr.openjdk.java.net/~smarks/reviews/6880112/bundle.1.hg
>
> If this doesn't meet with any objections, I'll merge these whitespace 
> changes and the earlier diamond conversion changes into a single 
> changeset and then push the result.
>

Looks good to me - approved.

-Joe




More information about the core-libs-dev mailing list