encoding/binary: fix error message

In the process, simplified internal sizeOf and
dataSize functions. Minor positive impact on
performance. Added test case.

benchmark                         old ns/op     new ns/op     delta
BenchmarkReadSlice1000Int32s      14006         14122         +0.83%
BenchmarkReadStruct               2508          2447          -2.43%
BenchmarkReadInts                 921           928           +0.76%
BenchmarkWriteInts                2086          2081          -0.24%
BenchmarkWriteSlice1000Int32s     13440         13497         +0.42%
BenchmarkPutUvarint32             28.5          26.3          -7.72%
BenchmarkPutUvarint64             81.3          76.7          -5.66%

benchmark                         old MB/s     new MB/s     speedup
BenchmarkReadSlice1000Int32s      285.58       283.24       0.99x
BenchmarkReadStruct               27.90        28.60        1.03x
BenchmarkReadInts                 32.57        32.31        0.99x
BenchmarkWriteInts                14.38        14.41        1.00x
BenchmarkWriteSlice1000Int32s     297.60       296.36       1.00x
BenchmarkPutUvarint32             140.55       151.92       1.08x
BenchmarkPutUvarint64             98.36        104.33       1.06x

Fixes #6818.

LGTM=r
R=r
CC=golang-codereviews
https://golang.org/cl/149290045

diff --git a/src/encoding/binary/binary.go b/src/encoding/binary/binary.go
index b5a377430f84bdd2e9eb2d659806e95a28f133f8..466bf97c973b573ed00301b671c06d9ca5b2c579 100644 (file)
--- a/src/encoding/binary/binary.go
+++ b/src/encoding/binary/binary.go
@@ -200,18 +200,17 @@ func Read(r io.Reader, order ByteOrder, data interface{}) error {
        }
 
        // Fallback to reflect-based decoding.
-       var v reflect.Value
-       switch d := reflect.ValueOf(data); d.Kind() {
+       v := reflect.ValueOf(data)
+       size := -1
+       switch v.Kind() {
        case reflect.Ptr:
-               v = d.Elem()
+               v = v.Elem()
+               size = dataSize(v)
        case reflect.Slice:
-               v = d
-       default:
-               return errors.New("binary.Read: invalid type " + d.Type().String())
+               size = dataSize(v)
        }
-       size, err := dataSize(v)
-       if err != nil {
-               return errors.New("binary.Read: " + err.Error())
+       if size < 0 {
+               return errors.New("binary.Read: invalid type " + reflect.TypeOf(data).String())
        }
        d := &decoder{order: order, buf: make([]byte, size)}
        if _, err := io.ReadFull(r, d.buf); err != nil {
@@ -324,68 +323,64 @@ func Write(w io.Writer, order ByteOrder, data interface{}) error {
 
        // Fallback to reflect-based encoding.
        v := reflect.Indirect(reflect.ValueOf(data))
-       size, err := dataSize(v)
-       if err != nil {
-               return errors.New("binary.Write: " + err.Error())
+       size := dataSize(v)
+       if size < 0 {
+               return errors.New("binary.Write: invalid type " + reflect.TypeOf(data).String())
        }
        buf := make([]byte, size)
        e := &encoder{order: order, buf: buf}
        e.value(v)
-       _, err = w.Write(buf)
+       _, err := w.Write(buf)
        return err
 }
 
 // Size returns how many bytes Write would generate to encode the value v, which
 // must be a fixed-size value or a slice of fixed-size values, or a pointer to such data.
+// If v is neither of these, Size returns -1.
 func Size(v interface{}) int {
-       n, err := dataSize(reflect.Indirect(reflect.ValueOf(v)))
-       if err != nil {
-               return -1
-       }
-       return n
+       return dataSize(reflect.Indirect(reflect.ValueOf(v)))
 }
 
 // dataSize returns the number of bytes the actual data represented by v occupies in memory.
 // For compound structures, it sums the sizes of the elements. Thus, for instance, for a slice
 // it returns the length of the slice times the element size and does not count the memory
-// occupied by the header.
-func dataSize(v reflect.Value) (int, error) {
+// occupied by the header. If the type of v is not acceptable, dataSize returns -1.
+func dataSize(v reflect.Value) int {
        if v.Kind() == reflect.Slice {
-               elem, err := sizeof(v.Type().Elem())
-               if err != nil {
-                       return 0, err
+               if s := sizeof(v.Type().Elem()); s >= 0 {
+                       return s * v.Len()
                }
-               return v.Len() * elem, nil
+               return -1
        }
        return sizeof(v.Type())
 }
 
-func sizeof(t reflect.Type) (int, error) {
+// sizeof returns the size >= 0 of variables for the given type or -1 if the type is not acceptable.
+func sizeof(t reflect.Type) int {
        switch t.Kind() {
        case reflect.Array:
-               n, err := sizeof(t.Elem())
-               if err != nil {
-                       return 0, err
+               if s := sizeof(t.Elem()); s >= 0 {
+                       return s * t.Len()
                }
-               return t.Len() * n, nil
 
        case reflect.Struct:
                sum := 0
                for i, n := 0, t.NumField(); i < n; i++ {
-                       s, err := sizeof(t.Field(i).Type)
-                       if err != nil {
-                               return 0, err
+                       s := sizeof(t.Field(i).Type)
+                       if s < 0 {
+                               return -1
                        }
                        sum += s
                }
-               return sum, nil
+               return sum
 
        case reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64,
                reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64,
                reflect.Float32, reflect.Float64, reflect.Complex64, reflect.Complex128:
-               return int(t.Size()), nil
+               return int(t.Size())
        }
-       return 0, errors.New("invalid type " + t.String())
+
+       return -1
 }
 
 type coder struct {
@@ -595,12 +590,11 @@ func (e *encoder) value(v reflect.Value) {
 }
 
 func (d *decoder) skip(v reflect.Value) {
-       n, _ := dataSize(v)
-       d.buf = d.buf[n:]
+       d.buf = d.buf[dataSize(v):]
 }
 
 func (e *encoder) skip(v reflect.Value) {
-       n, _ := dataSize(v)
+       n := dataSize(v)
        for i := range e.buf[0:n] {
                e.buf[i] = 0
        }

diff --git a/src/encoding/binary/binary_test.go b/src/encoding/binary/binary_test.go
index c80c90383afbeadb3cf30620f8e878cbfda4c293..8ee595fa476aabb677c87b08a065aec30b90af8c 100644 (file)
--- a/src/encoding/binary/binary_test.go
+++ b/src/encoding/binary/binary_test.go
@@ -289,6 +289,26 @@ func TestUnexportedRead(t *testing.T) {
        Read(&buf, LittleEndian, &u2)
 }
 
+func TestReadErrorMsg(t *testing.T) {
+       var buf bytes.Buffer
+       read := func(data interface{}) {
+               err := Read(&buf, LittleEndian, data)
+               want := "binary.Read: invalid type " + reflect.TypeOf(data).String()
+               if err == nil {
+                       t.Errorf("%T: got no error; want %q", data, want)
+                       return
+               }
+               if got := err.Error(); got != want {
+                       t.Errorf("%T: got %q; want %q", data, got, want)
+               }
+       }
+       read(0)
+       s := new(struct{})
+       read(&s)
+       p := &s
+       read(&p)
+}
+
 type byteSliceReader struct {
        remain []byte
 }
@@ -315,8 +335,7 @@ func BenchmarkReadStruct(b *testing.B) {
        bsr := &byteSliceReader{}
        var buf bytes.Buffer
        Write(&buf, BigEndian, &s)
-       n, _ := dataSize(reflect.ValueOf(s))
-       b.SetBytes(int64(n))
+       b.SetBytes(int64(dataSize(reflect.ValueOf(s))))
        t := s
        b.ResetTimer()
        for i := 0; i < b.N; i++ {