code review 7393075: strconv: Use Quote to escape the input string for faile... (issue 7393075)

[mdb...@google.com]

Reviewers: golang-dev_googlegroups.com,

Message:
Hello golan...@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/


Description:
strconv: Use Quote to escape the input string for failed conversion
errors.

This reveals the presence of control and non-printable characters in the
errors returned by the Parse functions.

Please review this at https://codereview.appspot.com/7393075/

Affected files:
M src/pkg/strconv/atof_test.go
M src/pkg/strconv/atoi.go
M src/pkg/strconv/atoi_test.go


Index: src/pkg/strconv/atof_test.go
===================================================================
--- a/src/pkg/strconv/atof_test.go
+++ b/src/pkg/strconv/atof_test.go
@@ -110,6 +110,7 @@
{"1e", "0", ErrSyntax},
{"1e-", "0", ErrSyntax},
{".e-1", "0", ErrSyntax},
+ {"1\x00.2", "0", ErrSyntax},

//
http://www.exploringbinary.com/java-hangs-when-converting-2-2250738585072012e-308/
{"2.2250738585072012e-308", "2.2250738585072014e-308", nil},
Index: src/pkg/strconv/atoi.go
===================================================================
--- a/src/pkg/strconv/atoi.go
+++ b/src/pkg/strconv/atoi.go
@@ -20,7 +20,7 @@
}

func (e *NumError) Error() string {
- return "strconv." + e.Func + ": " + `parsing "` + e.Num + `": ` +
e.Err.Error()
+ return "strconv." + e.Func + ": " + "parsing " + Quote(e.Num) + ": " +
e.Err.Error()
}

func syntaxError(fn, str string) *NumError {
Index: src/pkg/strconv/atoi_test.go
===================================================================
--- a/src/pkg/strconv/atoi_test.go
+++ b/src/pkg/strconv/atoi_test.go
@@ -5,6 +5,7 @@
package strconv_test

import (
+ "errors"
"reflect"
. "strconv"
"testing"
@@ -187,6 +188,22 @@
}
}

+func TestNumError(t *testing.T) {
+ for _, c := range []struct{ Num, Want string }{
+ {Num: "0", Want: `strconv.ParseFloat: parsing "0": failed`},
+ {Num: "1\x00.2", Want: `strconv.ParseFloat: parsing "1\x00.2": failed`},
+ } {
+ err := &NumError{
+ Func: "ParseFloat",
+ Num: c.Num,
+ Err: errors.New("failed"),
+ }
+ if got := err.Error(); got != c.Want {
+ t.Errorf(`&NumError{"ParseFloat", %q, "failed"} = %v, want %v`, c.Num,
got, c.Want)
+ }
+ }
+}
+
func TestParseUint64(t *testing.T) {
for i := range atoui64tests {
test := &atoui64tests[i]

[r...@golang.org]

https://codereview.appspot.com/7393075/diff/1001/src/pkg/strconv/atoi_test.go
File src/pkg/strconv/atoi_test.go (right):

https://codereview.appspot.com/7393075/diff/1001/src/pkg/strconv/atoi_test.go#newcode192
src/pkg/strconv/atoi_test.go:192: for _, c := range []struct{ Num, Want
string }{
the other tests here don't use this style. please be consistent.

https://codereview.appspot.com/7393075/

[Russ Cox]

Please call CanBackquote first and preserve the current `` quoting where possible.

[Matt Brown (mdbrown)]

Russ, do you mean something like

        if CanBackquote(e.Num) {

                quoted = "`" + e.Num + "`"

        } else {

                quoted = Quote(e.Num)

        }

?

[Russ Cox]

Yes, please. I think that will preserve the current string format except when there are backquotes or control characters.

[Matt Brown (mdbrown)]

I may not be following correctly but I think it has somewhat the opposite effect. Simple strings can be back-quoted, yielding test failures from other packages like

--- FAIL: TestNumberAccessors (0.00 seconds)

        decode_test.go:515: Number("-1.23e1").Int64() wanted error "strconv.ParseInt: parsing \"-1.23e1\": invalid syntax" but got: strconv.ParseInt: parsing `-1.23e1`: invalid syntax

        decode_test.go:515: Number("1e1000").Int64() wanted error "strconv.ParseInt: parsing \"1e1000\": invalid syntax" but got: strconv.ParseInt: parsing `1e1000`: invalid syntax

        decode_test.go:520: Number("1e1000").Float64() wanted error "strconv.ParseFloat: parsing \"1e1000\": value out of range" but got: strconv.ParseFloat: parsing `1e1000`: value out of range

FAIL

FAIL    encoding/json   0.023s

[Russ Cox]

My apologies for the wild goose chase. I misread this line:

[mdb...@google.com]

I've changed back to using a single call to Quote.

https://codereview.appspot.com/7393075/diff/1001/src/pkg/strconv/atoi_test.go
File src/pkg/strconv/atoi_test.go (right):

https://codereview.appspot.com/7393075/diff/1001/src/pkg/strconv/atoi_test.go#newcode192
src/pkg/strconv/atoi_test.go:192: for _, c := range []struct{ Num, Want
string }{

On 2013/02/27 21:32:47, r wrote:
> the other tests here don't use this style. please be consistent.

Done.

https://codereview.appspot.com/7393075/

[mdb...@google.com]

Hello golan...@googlegroups.com, r...@golang.org, r...@golang.org (cc:
golan...@googlegroups.com),

Please take another look.


https://codereview.appspot.com/7393075/

[r...@golang.org]

LGTM

https://codereview.appspot.com/7393075/

[r...@golang.org]

*** Submitted as
https://code.google.com/p/go/source/detail?r=7e7041319c25 ***

strconv: use Quote to escape the input string for failed conversion
errors

This reveals the presence of control and non-printable characters in the

errors returned by the Parse functions. Also add unit tests for
NumError.

R=golang-dev, r, rsc
CC=golang-dev
https://codereview.appspot.com/7393075

Committer: Rob Pike <r...@golang.org>


https://codereview.appspot.com/7393075/