On 11/17/2014 02:49 PM, Claes Redestad wrote: > On 2014-11-17 11:45, Aleksey Shipilev wrote: >> On 11/17/2014 02:09 AM, Claes Redestad wrote: >>> http://cr.openjdk.java.net/~redestad/8065070/webrev.00 >>> https://bugs.openjdk.java.net/browse/JDK-8065070 >> (Not a Reviewer) The change looks very sane given the capture ranges are >> already available in Matcher.I wonder if you want to cache m.start() >> and m.end() in locals while you are at it -- I would think this amounts >> to moving tTStart, tTEnd a few lines before. > > Thanks for taking a look at this! > > Not sure how you mean locally caching m.start(idx) would > improve things, as we never reuse the same start/end values.
Ah, right. Curse those inline side effects. > Perhaps rewriting to something like this would make the code > cleaner: > > index(s, m.start(1), m.end(1)); > flags(s, m.start(2), m.end(2)); > width(s, m.start(3), m.end(3)); > precision(s, m.start(4), m.end(4)); > > int tTStart = m.start(5); > if (tTStart >= 0) { > dt = true; > if (s.charAt(tTStart) == 'T') { > f.add(Flags.UPPERCASE); > } > } > conversion(s.charAt(m.start(6))); Yes please. > (I noticed that getting and checking tTEnd is basically redundant, > since the formatSpecifier regex guarantees either one char or > nothing is matched for that group) That makes sense. -Aleksey.