code review 4552056: big: make Int and Rat implement fmt.Scanner

big: make Int and Rat implement fmt.Scanner

Also add Len method to strings.Reader

[eds, 2011-05-20 21:37:43]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[eds, 2011-05-20 21:39:28]

I didn't add tests for Rat.Scan since it's pretty simple and would
essentially just be testing Rat.SetString, but please let me know if
you disagree.

- Evan

[gri, 2011-05-20 22:31:46]

Looks pretty good.

It would be nice if the scanners would actually accept the longest legal token,
and not just the longest non-space-terminated sequence of chars. It may make
this CL significantly bigger; if so, maybe it could be a follow-up CL.

Yes, please add the Rat test. I agree it's trivial, and you can probably copy
most of the structure, but it's always good to have the test, especially if we
make changes to this down the road.

http://codereview.appspot.com/4552056/diff/5001/src/pkg/big/int.go
File src/pkg/big/int.go (right):

http://codereview.appspot.com/4552056/diff/5001/src/pkg/big/int.go#newcode381
src/pkg/big/int.go:381: tok, err := s.Token(false, nil)
Can we make this accept real Go syntax? That is, consume the longest legal token
(depending on ch)? As is it, this will require space separated tokens and won't
parse correctly, say the 2 in 2+3.

http://codereview.appspot.com/4552056/diff/5001/src/pkg/big/int.go#newcode398
src/pkg/big/int.go:398: return os.NewError("Int.Scan: invalid verb")
s/NewError/ErrorString/

http://codereview.appspot.com/4552056/diff/5001/src/pkg/big/int.go#newcode401
src/pkg/big/int.go:401: return os.NewError("Int.Scan: invalid syntax")
same here

PS: There may be other occurrences in this file.

http://codereview.appspot.com/4552056/diff/5001/src/pkg/big/rat.go
File src/pkg/big/rat.go (right):

http://codereview.appspot.com/4552056/diff/5001/src/pkg/big/rat.go#newcode219
src/pkg/big/rat.go:219: tok, err := s.Token(false, nil)
Same here: Would be nice if this would accept the longest legal token. Maybe you
can shortcut if the next char is not in +-/0123456789.eE?

http://codereview.appspot.com/4552056/diff/5001/src/pkg/big/rat.go#newcode223
src/pkg/big/rat.go:223: switch ch {
if strings.IndexRune("efgEFGv", ch) < 0 {
  return os.ErrorString(...)
}

?

[eds, 2011-05-21 23:12:16]

I've uploaded a new patch, but it turned out kind of ugly after adding
the longest legal token stuff. I can't think of a way to make it
simpler without making it incorrect, but maybe you can offer a
suggestion or two.

PTAL

- Evan

[gri, 2011-05-24 00:44:51]

This is not too bad, I think.

It would be great if the rational scanner also would accept the longest legal
string only (at the moment, it may accept more and then bail with an error), but
that gets pretty complicated. I think this is fine for a start.

http://codereview.appspot.com/4552056/diff/12001/src/pkg/big/int.go
File src/pkg/big/int.go (right):

http://codereview.appspot.com/4552056/diff/12001/src/pkg/big/int.go#newcode329
src/pkg/big/int.go:329: case 'd', 'v':
case 'd', 's', 'v':

http://codereview.appspot.com/4552056/diff/12001/src/pkg/big/int.go#newcode349
src/pkg/big/int.go:349: case ch == 's':
remove this in favor of the 's' above

http://codereview.appspot.com/4552056/diff/12001/src/pkg/big/int.go#newcode395
src/pkg/big/int.go:395: case 'v':
's', 'v' ?

http://codereview.appspot.com/4552056/diff/12001/src/pkg/big/int.go#newcode404
src/pkg/big/int.go:404: neg := ch == '-'
neg := false
switch ch {
case '-': neg = true
case '+': // nothing to do
default: s.UnreadRune()
}

http://codereview.appspot.com/4552056/diff/12001/src/pkg/big/nat.go
File src/pkg/big/nat.go (right):

http://codereview.appspot.com/4552056/diff/12001/src/pkg/big/nat.go#newcode616
src/pkg/big/nat.go:616: case 'a' <= ch && ch <= 'f':
s/'f'/'z'/

since string(...) now also goes from 0..z

http://codereview.appspot.com/4552056/diff/12001/src/pkg/big/nat.go#newcode618
src/pkg/big/nat.go:618: case 'A' <= ch && ch <= 'F':
s/'F'/'Z'/

http://codereview.appspot.com/4552056/diff/12001/src/pkg/big/nat.go#newcode627
src/pkg/big/nat.go:627: // These constants are possible results of nat.scan.
I don't think these make a lot of sense with an io.Reader, since partial results
are the norm, failures are real errors, and situations where the scan exhaust
the reader are probably uncommon.

http://codereview.appspot.com/4552056/diff/12001/src/pkg/big/nat.go#newcode635
src/pkg/big/nat.go:635: // longest possible prefix of s representing a natural
number in a
s/of s/read from r/

http://codereview.appspot.com/4552056/diff/12001/src/pkg/big/nat.go#newcode636
src/pkg/big/nat.go:636: // given conversion base, the actual conversion base
used, and the
s/and the prefix length/and an error, if any/

http://codereview.appspot.com/4552056/diff/12001/src/pkg/big/nat.go#newcode645
src/pkg/big/nat.go:645: func (z nat) scan(r io.RuneReader, base int) (nat, int,
int) {
s/(int, int, int)/(int, int, os.Error)/

http://codereview.appspot.com/4552056/diff/12001/src/pkg/big/nat.go#newcode648
src/pkg/big/nat.go:648: return z, 0, scanFail
return z, 0, err

http://codereview.appspot.com/4552056/diff/12001/src/pkg/big/nat.go#newcode653
src/pkg/big/nat.go:653: if ch == '0' {
if ch, _, err = r.ReadRune(); err {
case nil:
  // code for the no error case
case os.EOF:
  z = z.make(0)
  return z, 10, nil
default:
  return z, 0, err
}

http://codereview.appspot.com/4552056/diff/12001/src/pkg/big/nat.go#newcode669
src/pkg/big/nat.go:669: return z, 0, scanFail
return z, 0, os.ErrorString("syntax error scanning binary or hexadecimal
number")

(analogous to fmt/scan.go:429)

http://codereview.appspot.com/4552056/diff/12001/src/pkg/big/nat.go#newcode677
src/pkg/big/nat.go:677: if base < 2 || 16 < base {
s/16/'z' - 'a' + 10/

http://codereview.appspot.com/4552056/diff/12001/src/pkg/big/nat.go#newcode678
src/pkg/big/nat.go:678: return z, 0, scanFail
os.ErrorString("illegal number base")

[eds, 2011-05-24 07:35:00]

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

Please take another look.

[eds, 2011-05-24 07:35:38]

All comments addressed, but please see one response below.

http://codereview.appspot.com/4552056/diff/12001/src/pkg/big/nat.go
File src/pkg/big/nat.go (right):

http://codereview.appspot.com/4552056/diff/12001/src/pkg/big/nat.go#newcode627
src/pkg/big/nat.go:627: // These constants are possible results of nat.scan.
On 2011/05/24 00:44:51, gri wrote:
> I don't think these make a lot of sense with an io.Reader, since partial
results
> are the norm, failures are real errors, and situations where the scan exhaust
> the reader are probably uncommon.

Doesn't Int.Scan need to know whether the whole input was consumed? Otherwise,
it will have to always call s.UnreadRune when err == nil. Most of the time this
is correct, but if we've exhausted the reader, won't this leave behind an extra
rune that we actually used?

[gri, 2011-05-24 17:10:47]

Getting there.

Regarding your comment (use of UnreadRun): I believe you are correct.

I think the UnreadRune simply has to go into the scan function, where you have
the information needed and thus can call it only if necessary.

There should be extra tests (or the table entries for the existing test need to
be expanded) so that these things are tested, too (state of the reader).

http://codereview.appspot.com/4552056/diff/2003/src/pkg/big/int.go
File src/pkg/big/int.go (right):

http://codereview.appspot.com/4552056/diff/2003/src/pkg/big/int.go#newcode397
src/pkg/big/int.go:397: ch, _, err := s.ReadRune()
Since you use empty lines for grouping inside this function: It could use an
empty line before 397. (This is the group of statements concerned with the
sign).

http://codereview.appspot.com/4552056/diff/2003/src/pkg/big/int.go#newcode414
src/pkg/big/int.go:414: s.UnreadRune()
This may have to move into scan().

http://codereview.appspot.com/4552056/diff/2003/src/pkg/big/nat.go
File src/pkg/big/nat.go (right):

http://codereview.appspot.com/4552056/diff/2003/src/pkg/big/nat.go#newcode680
src/pkg/big/nat.go:680: } else if len(z) > 0 {
Actually, this is not correct: If the input is "0 " (note the blank after the 0)
and base == 10, z is 0, and len(z) == 0, and we get a syntax error even though
it was a correct 0.

http://codereview.appspot.com/4552056/diff/2003/src/pkg/big/nat.go#newcode684
src/pkg/big/nat.go:684: return z, 0, os.ErrorString("syntax error")
perhaps: syntax error scanning number

http://codereview.appspot.com/4552056/diff/2003/src/pkg/big/nat_test.go
File src/pkg/big/nat_test.go (right):

http://codereview.appspot.com/4552056/diff/2003/src/pkg/big/nat_test.go#newco...
src/pkg/big/nat_test.go:191: s := a.x.string(a.c)
The tests don't exercise the case where base = 0. Also, there need to be tests
that don't end in EOF. For instance scanning "0 " with base 0 doesn't work
correctly at the moment.

http://codereview.appspot.com/4552056/diff/2003/src/pkg/big/nat_test.go#newco...
src/pkg/big/nat_test.go:197: x, b, err := nat(nil).scan(&r, len(a.c))
s/&r/strings.NewReader(a.s)

and get rid of the r

[eds, 2011-05-24 20:19:13]

On Wed, May 25, 2011 at 5:10 AM,  <gri@golang.org> wrote:
> I think the UnreadRune simply has to go into the scan function, where
> you have the information needed and thus can call it only if necessary.

It would be nice to call UnreadRune in nat.scan, but strings.Reader
doesn't have UnreadRune. Int.SetString doesn't actually need to unread
runes since it's all or nothing, but it wouldn't hurt if it were able
to.

I'm hesitant to add UnreadRune to strings.Reader because it will
require a fairly big change to that type, but if you or anyone else
thinks it's worthwhile, I can go that route.

- Evan

[gri, 2011-05-24 22:21:02]

Ah, I missed that. This is unfortunate.

scan could return an additional argument (the next char) but that
makes it even more complicated. I don't think the current split is
right: (nat) scan should either be able to cleanly use a RuneReader,
or we have to find another approach.

I need to think about this some more.
- gri

On Tue, May 24, 2011 at 1:19 PM, Evan Shaw <chickencha@gmail.com> wrote:
> It would be nice to call UnreadRune in nat.scan, but strings.Reader
> doesn't have UnreadRune. Int.SetString doesn't actually need to unread
> runes since it's all or nothing, but it wouldn't hurt if it were able
> to.
>
> I'm hesitant to add UnreadRune to strings.Reader because it will
> require a fairly big change to that type, but if you or anyone else
> thinks it's worthwhile, I can go that route.

[eds, 2011-05-24 22:43:07]

On Wed, May 25, 2011 at 10:20 AM, Robert Griesemer <gri@golang.org> wrote:
> scan could return an additional argument (the next char) but that
> makes it even more complicated. I don't think the current split is
> right: (nat) scan should either be able to cleanly use a RuneReader,
> or we have to find another approach.

Another approach I tried but didn't submit was passing a closure to
ScanState.Token. The closure kept track of the state of the scanner,
returning true or false as appropriate. I decided that was worse than
the current approach, though.

- Evan

[eds, 2011-05-27 10:14:24]

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

Please take another look.

[gri, 2011-05-27 21:35:36]

Looking pretty good. It would be nice to have a few more test cases; and I have
some simplification suggestions.

- gri

http://codereview.appspot.com/4552056/diff/19003/src/pkg/big/int.go
File src/pkg/big/int.go (right):

http://codereview.appspot.com/4552056/diff/19003/src/pkg/big/int.go#newcode463
src/pkg/big/int.go:463: return z, err == nil && r.Len() == 0
The logic in this function can be simplified a bit because we have error
handling in scan. I suggest going w/o the new Len() function in strings.Reader;
that will make it possible to have a Scan function for Ints, similarly to scan
for nats - well, it will have to have a different name to avoid conflicts, but
still (not in this CL).


func (z *Int) SetString(s string, base int) (*Int, bool) {
	neg := false
	if len(s) > 0 {
		switch s[0] {
		case '-':
			neg = true
			fallthrough
		case '+':
			s = s[1:]
		}
	}

	r := strings.NewReader(s)
	abs, _, err := z.abs.scan(r, base)
	if err != nil {
		return z, false
	}
	_, _, err = r.ReadRune()

	z.abs = abs
	z.neg = len(abs) > 0 && neg // 0 has no sign
	return z, err == os.EOF // err == os.EOF => scan consumed all of s
}

http://codereview.appspot.com/4552056/diff/19003/src/pkg/big/int_test.go
File src/pkg/big/int_test.go (right):

http://codereview.appspot.com/4552056/diff/19003/src/pkg/big/int_test.go#newc...
src/pkg/big/int_test.go:415: {"-A", "%X", "-10", 0},
add a case with +

http://codereview.appspot.com/4552056/diff/19003/src/pkg/big/nat.go
File src/pkg/big/nat.go (right):

http://codereview.appspot.com/4552056/diff/19003/src/pkg/big/nat.go#newcode685
src/pkg/big/nat.go:685: if n > 0 {
if n > 0 {
   break
}
return ... // error

http://codereview.appspot.com/4552056/diff/19003/src/pkg/big/nat_test.go
File src/pkg/big/nat_test.go (right):

http://codereview.appspot.com/4552056/diff/19003/src/pkg/big/nat_test.go#newc...
src/pkg/big/nat_test.go:227: {"0XDEADBEEF", nat{0xdeadbeef}, 16, true},
please add some examples with bases > 16

http://codereview.appspot.com/4552056/diff/19003/src/pkg/strings/reader.go
File src/pkg/strings/reader.go (right):

http://codereview.appspot.com/4552056/diff/19003/src/pkg/strings/reader.go#ne...
src/pkg/strings/reader.go:84: // Len returns the number of bytes of the unread
portion of the buffer.
This is fine, but I would make it a separate CL if you think this is a useful
function. It's not absolutely needed for this CL (see my implementation
suggestion).

[eds, 2011-05-27 21:59:02]

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

Please take another look.

[gri, 2011-05-27 22:47:14]

LGTM
thanks for doing this.
- gri

On Fri, May 27, 2011 at 2:59 PM,  <chickencha@gmail.com> wrote:
> Hello gri@golang.org (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
> http://codereview.appspot.com/4552056/
>

[gri, 2011-05-27 22:51:04]

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

big: make Int and Rat implement fmt.Scanner

R=gri
CC=golang-dev
http://codereview.appspot.com/4552056

Committer: Robert Griesemer <gri@golang.org>