code review 1502041: fmt.Scan: field widths

fmt.Scan: field widths
Also fix an interface bug: white-space-delimited doesn't work well for cases like "%d, %d" on "23, 23")

[r, 2010-06-03 06:01:48]

Hello rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[r2, 2010-06-03 07:08:30]

Please hold off. I have a simplification I want to try in the morning.

-rob

[r, 2010-06-03 18:27:31]

Hello rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc1, 2010-06-03 18:56:36]

Does this work or is it an error?

var (i int; s string)
Sscan("123abc", &i, &s)

Probably want a test either way.

http://codereview.appspot.com/1502041/diff/13001/14002
File src/pkg/fmt/scan.go (right):

http://codereview.appspot.com/1502041/diff/13001/14002#newcode320
src/pkg/fmt/scan.go:320: if !!unicode.IsSpace(rune) {
!!?

http://codereview.appspot.com/1502041/diff/13001/14002#newcode507
src/pkg/fmt/scan.go:507: if !s.accept("+-") {
for later: should complex accept "1i" and "3.14" ?

http://codereview.appspot.com/1502041/diff/13001/14003
File src/pkg/fmt/scan_test.go (right):

http://codereview.appspot.com/1502041/diff/13001/14003#newcode288
src/pkg/fmt/scan_test.go:288: var multiTests = []ScanfMultiTest{
func z(args ... interface{}) []interface{} { return args }
(there's probably a better name)

ScanfMultiTest{"%2d%3d", "44555", z(&i, &j), z(4, 555), ""}

http://codereview.appspot.com/1502041/diff/13001/14003#newcode390
src/pkg/fmt/scan_test.go:390: if err != nil {
should check err == nil (make sure test.err == "") too

http://codereview.appspot.com/1502041/diff/13001/14004
File src/pkg/netchan/netchan_test.go (right):

http://codereview.appspot.com/1502041/diff/13001/14004#newcode53
src/pkg/netchan/netchan_test.go:53: t.Errorf("expected close at %d; got one at
%d\n", closeCount, i)
Fine but seems unrelated.

[r2, 2010-06-03 19:21:24]

On Jun 3, 2010, at 11:56 AM, rsc@google.com wrote:

> Does this work or is it an error?
> 
> var (i int; s string)
> Sscan("123abc", &i, &s)
> 
> Probably want a test either way.

it works.  i can easily make it not work by forcing %v to be space separated. 
an interesting related case is

Sscanf("%o%d", "12345999", &i, &j).

my current rules are that it scans the longest legal string for the format,
which is an easy rule.  i could change it so numbers scan maximum *digits*, so
this case would be an error.  Opinions welcome.

> 
> 
> 
> http://codereview.appspot.com/1502041/diff/13001/14002
> File src/pkg/fmt/scan.go (right):
> 
> http://codereview.appspot.com/1502041/diff/13001/14002#newcode320
> src/pkg/fmt/scan.go:320: if !!unicode.IsSpace(rune) {
> !!?

editing amazement.

> 
> http://codereview.appspot.com/1502041/diff/13001/14002#newcode507
> src/pkg/fmt/scan.go:507: if !s.accept("+-") {
> for later: should complex accept "1i" and "3.14" ?

yes. added TODO.

> http://codereview.appspot.com/1502041/diff/13001/14003
> File src/pkg/fmt/scan_test.go (right):
> 
> http://codereview.appspot.com/1502041/diff/13001/14003#newcode288
> src/pkg/fmt/scan_test.go:288: var multiTests = []ScanfMultiTest{
> func z(args ... interface{}) []interface{} { return args }
> (there's probably a better name)
> 
> ScanfMultiTest{"%2d%3d", "44555", z(&i, &j), z(4, 555), ""}

nice. done.

> http://codereview.appspot.com/1502041/diff/13001/14003#newcode390
> src/pkg/fmt/scan_test.go:390: if err != nil {
> should check err == nil (make sure test.err == "") too

done.

[rsc, 2010-06-03 20:34:16]

> it works.  i can easily make it not work by forcing %v to be space separated.
 an interesting related case is
>
> Sscanf("%o%d", "12345999", &i, &j).
>
> my current rules are that it scans the longest legal string for the format,
which is an easy rule.  i could change it so numbers scan maximum *digits*, so
this case would be an error.  Opinions welcome.

reading scanf(3), it sounds like your current rules and
C scanf agree, so that's probably good.

russ

[r2, 2010-06-03 20:58:02]

any other comments?

[rsc, 2010-06-03 22:21:32]

LGTM

[r, 2010-06-03 23:06:44]

*** Submitted as http://code.google.com/p/go/source/detail?r=90a03a765b92 ***

fmt.Scan: field widths
Also fix an interface bug: white-space-delimited doesn't work well for cases
like "%d, %d" on "23, 23")

R=rsc
CC=golang-dev
http://codereview.appspot.com/1502041