On Thu, Nov 5, 2009 at 15:22, David Schlosnagle <schlo...@gmail.com> wrote: > Martin, > > Did you want to remove the commented out formatter? > > //private Formatter formatter;
Yes, thanks, a mercurial glitch. Webrev already updated. http://cr.openjdk.java.net/~martin/webrevs/openjdk7/Formatter.parse/ Martin > > - Dave Schlosnagle > > > On Thu, Nov 5, 2009 at 6:14 PM, Martin Buchholz <marti...@google.com> wrote: >> On Wed, Nov 4, 2009 at 23:07, Xueming Shen <xueming.s...@sun.com> wrote: >>> #2659: >>> >>> conversion(m.group(idx++)); >>> >>> -> conversion(m.group(idx)); ? >> >> Done. >> >> I couldn't help making this additonal change: >> >> diff --git a/src/share/classes/java/util/Formatter.java >> b/src/share/classes/java/util/Formatter.java >> --- a/src/share/classes/java/util/Formatter.java >> +++ b/src/share/classes/java/util/Formatter.java >> @@ -2503,7 +2503,7 @@ >> al.add(new FixedString(s.substring(i, m.start()))); >> } >> >> - al.add(new FormatSpecifier(this, m)); >> + al.add(new FormatSpecifier(m)); >> i = m.end(); >> } else { >> // No more valid format specifiers. Check for possible >> invalid >> @@ -2552,7 +2552,7 @@ >> private boolean dt = false; >> private char c; >> >> - private Formatter formatter; >> + //private Formatter formatter; >> >> // cache the line separator >> private String ls; >> @@ -2640,8 +2640,7 @@ >> return c; >> } >> >> - FormatSpecifier(Formatter formatter, Matcher m) { >> - this.formatter = formatter; >> + FormatSpecifier(Matcher m) { >> int idx = 1; >> >> index(m.group(idx++)); >> @@ -2656,7 +2655,7 @@ >> f.add(Flags.UPPERCASE); >> } >> >> - conversion(m.group(idx++)); >> + conversion(m.group(idx)); >> >> if (dt) >> checkDateTime(); >> @@ -2811,9 +2810,9 @@ >> >> private void printString(Object arg, Locale l) throws IOException { >> if (arg instanceof Formattable) { >> - Formatter fmt = formatter; >> - if (formatter.locale() != l) >> - fmt = new Formatter(formatter.out(), l); >> + Formatter fmt = Formatter.this; >> + if (fmt.locale() != l) >> + fmt = new Formatter(fmt.out(), l); >> ((Formattable)arg).formatTo(fmt, f.valueOf(), width, >> precision); >> } else { >> if (f.contains(Flags.ALTERNATE)) >> >> FormatConversion is an inner class, so already has >> a pointer to its enclosing instance - no need to provide >> an additional explicit one. >> >> Martin >> >>> >>> >>> The rest looks fine. >>> >>> sherman >>> >>> >>> Martin Buchholz wrote: >>>> >>>> Hi Sherman, >>>> >>>> I'd like you to do a code review. >>>> >>>> http://cr.openjdk.java.net/~martin/webrevs/openjdk7/Formatter.parse/ >>>> <http://cr.openjdk.java.net/%7Emartin/webrevs/openjdk7/Formatter.parse/> >>>> >>>> Synopsis: Optimize Formatter.parse (including String.printf) >>>> Description: >>>> Formatter is not as efficient as it could be. >>>> Here's a low-hanging fruit optimization >>>> that creates fewer objects, >>>> and uses the idiom >>>> return al.toArray(new FormatString[al.size()]); >>>> I'm sure additional optimizations are possible. >>>> >>>> Results: about 10-20% faster on in-house microbenchmarks of >>>> String.printf >>>> >>>> Martin >>> >>> >> >
