]> Cypherpunks repositories - gostls13.git/commitdiff
fmt: cleanup reflect value handling
authorMartin Möhrmann <martisch@uos.de>
Sat, 19 Mar 2016 07:54:07 +0000 (08:54 +0100)
committerRob Pike <r@golang.org>
Sun, 27 Mar 2016 00:50:31 +0000 (00:50 +0000)
Merge printReflectValue into printValue. Determine if handleMethods
was already called in printArg by checking if depth is 0. Do not
call handleMethods on depth 0 again in printValue to not introduce
a performance regression. handleMethods is called already in printArg
to not introduce a performance penalty for top-level Stringer,
GoStringer, Errors and Formatters by using reflect.ValueOf on them
just to retrieve them again as interface{} values in printValue.

Clear p.arg in printValue after handleMethods to print the type
of the value inside the reflect.Value when a bad verb is encountered
on the top level instead of printing "reflect.Value=" as the type of
the argument. This also fixes a bug that incorrectly prints the
whole map instead of just the value for a key if the returned value
by the map for the key is an invalid reflect value.

name                     old time/op  new time/op  delta
SprintfPadding-2          229ns ± 2%   227ns ± 1%  -0.50%  (p=0.013 n=20+20)
SprintfEmpty-2           36.4ns ± 6%  37.2ns ±14%    ~     (p=0.091 n=18+20)
SprintfString-2           102ns ± 1%   102ns ± 0%    ~     (p=0.751 n=20+20)
SprintfTruncateString-2   142ns ± 0%   141ns ± 1%  -0.95%  (p=0.000 n=16+20)
SprintfQuoteString-2      389ns ± 0%   388ns ± 0%  -0.12%  (p=0.019 n=20+20)
SprintfInt-2              100ns ± 2%   100ns ± 1%    ~     (p=0.188 n=20+15)
SprintfIntInt-2           155ns ± 3%   154ns ± 2%    ~     (p=0.092 n=20+20)
SprintfPrefixedInt-2      250ns ± 2%   251ns ± 3%    ~     (p=0.559 n=20+20)
SprintfFloat-2            177ns ± 2%   175ns ± 1%  -1.30%  (p=0.000 n=20+20)
SprintfComplex-2          516ns ± 1%   510ns ± 1%  -1.13%  (p=0.000 n=19+16)
SprintfBoolean-2         90.9ns ± 3%  90.6ns ± 1%    ~     (p=0.193 n=19+19)
SprintfHexString-2        171ns ± 1%   169ns ± 1%  -1.44%  (p=0.000 n=19+20)
SprintfHexBytes-2         180ns ± 1%   180ns ± 1%    ~     (p=0.060 n=19+18)
SprintfBytes-2            330ns ± 1%   329ns ± 1%  -0.42%  (p=0.003 n=20+20)
SprintfStringer-2         354ns ± 3%   352ns ± 3%    ~     (p=0.525 n=20+19)
SprintfStructure-2        804ns ± 3%   776ns ± 2%  -3.56%  (p=0.000 n=20+20)
FprintInt-2               155ns ± 0%   151ns ± 1%  -2.35%  (p=0.000 n=19+20)
FprintfBytes-2            169ns ± 0%   170ns ± 1%  +0.81%  (p=0.000 n=18+19)
FprintIntNoAlloc-2        112ns ± 0%   109ns ± 1%  -2.28%  (p=0.000 n=20+20)

Change-Id: Ib9a39082ed1be0f1f7499ee6fb6c9530f043e43a
Reviewed-on: https://go-review.googlesource.com/20923
Run-TryBot: Rob Pike <r@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
src/fmt/fmt_test.go
src/fmt/print.go

index be7299cdbc412c121015e402d9ed8d7d396dac8a..ffa24995a539e6cca06247645e0efff7263f2927 100644 (file)
@@ -977,6 +977,8 @@ var fmtTests = []struct {
 
        // invalid reflect.Value doesn't crash.
        {"%v", reflect.Value{}, "<invalid reflect.Value>"},
+       {"%v", &reflect.Value{}, "<invalid Value>"},
+       {"%v", SI{reflect.Value{}}, "{<invalid Value>}"},
 
        // Tests to check that not supported verbs generate an error string.
        {"%☠", nil, "%!☠(<nil>)"},
@@ -995,6 +997,12 @@ var fmtTests = []struct {
        {"%☠", &intVar, "%!☠(*int=0xPTR)"},
        {"%☠", make(chan int), "%!☠(chan int=0xPTR)"},
        {"%☠", func() {}, "%!☠(func()=0xPTR)"},
+       {"%☠", reflect.ValueOf(renamedInt(0)), "%!☠(fmt_test.renamedInt=0)"},
+       {"%☠", SI{renamedInt(0)}, "{%!☠(fmt_test.renamedInt=0)}"},
+       {"%☠", &[]interface{}{I(1), G(2)}, "&[%!☠(fmt_test.I=1) %!☠(fmt_test.G=2)]"},
+       {"%☠", SI{&[]interface{}{I(1), G(2)}}, "{%!☠(*[]interface {}=&[1 2])}"},
+       {"%☠", reflect.Value{}, "<invalid reflect.Value>"},
+       {"%☠", map[float64]int{NaN: 1}, "map[%!☠(float64=NaN):%!☠(<nil>)]"},
 }
 
 // zeroFill generates zero-filled strings of the specified width. The length
@@ -1262,6 +1270,24 @@ func BenchmarkSprintfBytes(b *testing.B) {
        })
 }
 
+func BenchmarkSprintfStringer(b *testing.B) {
+       stringer := I(12345)
+       b.RunParallel(func(pb *testing.PB) {
+               for pb.Next() {
+                       Sprintf("%v", stringer)
+               }
+       })
+}
+
+func BenchmarkSprintfStructure(b *testing.B) {
+       s := &[]interface{}{SI{12345}, map[int]string{0: "hello"}}
+       b.RunParallel(func(pb *testing.PB) {
+               for pb.Next() {
+                       Sprintf("%#v", s)
+               }
+       })
+}
+
 func BenchmarkManyArgs(b *testing.B) {
        b.RunParallel(func(pb *testing.PB) {
                var buf bytes.Buffer
index c80223269ba2d0711840adfef800e5a038693dba..6c64773a1babf56cb869d1402047c1e18ec9158b 100644 (file)
@@ -657,57 +657,44 @@ func (p *pp) printArg(arg interface{}, verb rune) {
        case []byte:
                p.fmtBytes(f, verb, bytesString)
        case reflect.Value:
-               p.printReflectValue(f, verb, 0)
-               return
+               p.printValue(f, verb, 0)
        default:
                // If the type is not simple, it might have methods.
-               if p.handleMethods(verb) {
-                       return
+               if !p.handleMethods(verb) {
+                       // Need to use reflection, since the type had no
+                       // interface methods that could be used for formatting.
+                       p.printValue(reflect.ValueOf(f), verb, 0)
                }
-               // Need to use reflection
-               p.printReflectValue(reflect.ValueOf(arg), verb, 0)
-               return
        }
-       p.arg = nil
 }
 
+var byteType = reflect.TypeOf(byte(0))
+
 // printValue is similar to printArg but starts with a reflect value, not an interface{} value.
 // It does not handle 'p' and 'T' verbs because these should have been already handled by printArg.
 func (p *pp) printValue(value reflect.Value, verb rune, depth int) {
-       if !value.IsValid() {
-               switch verb {
-               case 'v':
-                       p.buf.WriteString(nilAngleString)
-               default:
-                       p.badVerb(verb)
-               }
-               return
-       }
-
-       // Handle values with special methods.
-       // Call always, even when arg == nil, because handleMethods clears p.fmt.plus for us.
-       p.arg = nil // Make sure it's cleared, for safety.
-       if value.CanInterface() {
+       // Handle values with special methods if not already handled by printArg (depth == 0).
+       if depth > 0 && value.IsValid() && value.CanInterface() {
                p.arg = value.Interface()
+               if p.handleMethods(verb) {
+                       return
+               }
        }
-       if p.handleMethods(verb) {
-               return
-       }
-
-       p.printReflectValue(value, verb, depth)
-}
-
-var byteType = reflect.TypeOf(byte(0))
-
-// printReflectValue is the fallback for both printArg and printValue.
-// It uses reflect to print the value.
-func (p *pp) printReflectValue(value reflect.Value, verb rune, depth int) {
-       oldValue := p.value
+       p.arg = nil
        p.value = value
-BigSwitch:
-       switch f := value; f.Kind() {
+
+       switch f := value; value.Kind() {
        case reflect.Invalid:
-               p.buf.WriteString(invReflectString)
+               if depth == 0 {
+                       p.buf.WriteString(invReflectString)
+               } else {
+                       switch verb {
+                       case 'v':
+                               p.buf.WriteString(nilAngleString)
+                       default:
+                               p.badVerb(verb)
+                       }
+               }
        case reflect.Bool:
                p.fmtBool(f.Bool(), verb)
        case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
@@ -729,7 +716,7 @@ BigSwitch:
                        p.buf.WriteString(f.Type().String())
                        if f.IsNil() {
                                p.buf.WriteString(nilParenString)
-                               break
+                               return
                        }
                        p.buf.WriteByte('{')
                } else {
@@ -755,12 +742,10 @@ BigSwitch:
                }
        case reflect.Struct:
                if p.fmt.sharpV {
-                       p.buf.WriteString(value.Type().String())
+                       p.buf.WriteString(f.Type().String())
                }
                p.buf.WriteByte('{')
-               v := f
-               t := v.Type()
-               for i := 0; i < v.NumField(); i++ {
+               for i := 0; i < f.NumField(); i++ {
                        if i > 0 {
                                if p.fmt.sharpV {
                                        p.buf.WriteString(commaSpaceString)
@@ -769,12 +754,12 @@ BigSwitch:
                                }
                        }
                        if p.fmt.plusV || p.fmt.sharpV {
-                               if f := t.Field(i); f.Name != "" {
-                                       p.buf.WriteString(f.Name)
+                               if name := f.Type().Field(i).Name; name != "" {
+                                       p.buf.WriteString(name)
                                        p.buf.WriteByte(':')
                                }
                        }
-                       p.printValue(getField(v, i), verb, depth+1)
+                       p.printValue(getField(f, i), verb, depth+1)
                }
                p.buf.WriteByte('}')
        case reflect.Interface:
@@ -813,13 +798,13 @@ BigSwitch:
                                }
                        }
                        p.fmtBytes(bytes, verb, typ.String())
-                       break
+                       return
                }
                if p.fmt.sharpV {
                        p.buf.WriteString(typ.String())
                        if f.Kind() == reflect.Slice && f.IsNil() {
                                p.buf.WriteString(nilParenString)
-                               break
+                               return
                        }
                        p.buf.WriteByte('{')
                } else {
@@ -841,32 +826,22 @@ BigSwitch:
                        p.buf.WriteByte(']')
                }
        case reflect.Ptr:
-               v := f.Pointer()
                // pointer to array or slice or struct?  ok at top level
                // but not embedded (avoid loops)
-               if v != 0 && depth == 0 {
+               if depth == 0 && f.Pointer() != 0 {
                        switch a := f.Elem(); a.Kind() {
-                       case reflect.Array, reflect.Slice:
+                       case reflect.Array, reflect.Slice, reflect.Struct, reflect.Map:
                                p.buf.WriteByte('&')
                                p.printValue(a, verb, depth+1)
-                               break BigSwitch
-                       case reflect.Struct:
-                               p.buf.WriteByte('&')
-                               p.printValue(a, verb, depth+1)
-                               break BigSwitch
-                       case reflect.Map:
-                               p.buf.WriteByte('&')
-                               p.printValue(a, verb, depth+1)
-                               break BigSwitch
+                               return
                        }
                }
                fallthrough
        case reflect.Chan, reflect.Func, reflect.UnsafePointer:
-               p.fmtPointer(value, verb)
+               p.fmtPointer(f, verb)
        default:
                p.unknownType(f)
        }
-       p.value = oldValue
 }
 
 // intFromArg gets the argNumth element of a. On return, isInt reports whether the argument has integer type.