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 issue 6818.

LGTM=r
R=r
CC=golang-codereviews
https://codereview.appspot.com/149290045

2 files changed, 53 insertions, 40 deletions

diff --git a/src/encoding/binary/binary.go b/src/encoding/binary/binary.go
index b5a377430..466bf97c9 100644
--- 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 c80c90383..8ee595fa4 100644
--- 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++ {