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.
