code review 4240056: fmt: allow recursive calls to Fscan etc.

fmt: allow recursive calls to Fscan etc.
Add a new Read method to ScanState so that it
satisfies the io.Reader interface; rename
Getrune and Ungetrune to ReadRune and UnreadRune.
Make sure ReadRune does not read past width restrictions;
remove now-unnecessary Width method from ScanState.
Also make the documentation a little clearer as to
how ReadRune and UnreadRune are used.

[rog, 2011-03-01 16:07:30]

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

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

[r, 2011-03-01 19:27:18]

i see why your tests aren't catching the bug: you have no tests

i warn you that doing this correctly is not this easy.

http://codereview.appspot.com/4240056/diff/4002/src/pkg/fmt/doc.go
File src/pkg/fmt/doc.go (right):

http://codereview.appspot.com/4240056/diff/4002/src/pkg/fmt/doc.go#newcode167
src/pkg/fmt/doc.go:167: Note: Fscan etc.  can read one character (rune) past the
input
one space

http://codereview.appspot.com/4240056/diff/4002/src/pkg/fmt/scan.go
File src/pkg/fmt/scan.go (right):

http://codereview.appspot.com/4240056/diff/4002/src/pkg/fmt/scan.go#newcode44
src/pkg/fmt/scan.go:44: // Read is provided only so that a ScanState can be
passed
s/only //

http://codereview.appspot.com/4240056/diff/4002/src/pkg/fmt/scan.go#newcode157
src/pkg/fmt/scan.go:157: r, _, err := s.ReadRune()
this design is problematic in handling partial runes.

http://codereview.appspot.com/4240056/diff/4002/src/pkg/fmt/scan.go#newcode169
src/pkg/fmt/scan.go:169: buf = buf[rlen:]
where does the rune go into the buffer? your tests can't be working.

[rog, 2011-03-02 14:23:29]

PTAL.

i've removed the Width field as it now seems redundant; the Token method could
also go now, as it's now equivalent to Scan(state, &s), but efficiency concerns
might rule this out.

http://codereview.appspot.com/4240056/diff/4002/src/pkg/fmt/doc.go
File src/pkg/fmt/doc.go (right):

http://codereview.appspot.com/4240056/diff/4002/src/pkg/fmt/doc.go#newcode167
src/pkg/fmt/doc.go:167: Note: Fscan etc.  can read one character (rune) past the
input
On 2011/03/01 19:27:18, r wrote:
> one space

ah... funnily enough the responsibility for that lies with some other code of
yours, also named fmt.

[r2, 2011-03-02 18:07:43]

On Mar 2, 2011, at 6:23 AM, rogpeppe@gmail.com wrote:

> PTAL.
> 
> i've removed the Width field as it now seems redundant; the Token method
> could also go now, as it's now equivalent to Scan(state, &s), but
> efficiency concerns might rule this out.
> 
> 
> 
> http://codereview.appspot.com/4240056/diff/4002/src/pkg/fmt/doc.go
> File src/pkg/fmt/doc.go (right):
> 
> http://codereview.appspot.com/4240056/diff/4002/src/pkg/fmt/doc.go#newcode167
> src/pkg/fmt/doc.go:167: Note: Fscan etc.  can read one character (rune)
> past the input
> On 2011/03/01 19:27:18, r wrote:
>> one space
> 
> ah... funnily enough the responsibility for that lies with some other
> code of yours, also named fmt.

my memory may be wrong but i believe td wrote fmt. all i added was retaining
indentation.

-rob

[rog, 2011-03-02 18:10:05]

On 2 March 2011 18:07, Rob 'Commander' Pike <r@google.com> wrote:
>
> On Mar 2, 2011, at 6:23 AM, rogpeppe@gmail.com wrote:
>
>> PTAL.
>>
>> i've removed the Width field as it now seems redundant; the Token method
>> could also go now, as it's now equivalent to Scan(state, &s), but
>> efficiency concerns might rule this out.
>>
>>
>>
>> http://codereview.appspot.com/4240056/diff/4002/src/pkg/fmt/doc.go
>> File src/pkg/fmt/doc.go (right):
>>
>> http://codereview.appspot.com/4240056/diff/4002/src/pkg/fmt/doc.go#newcode167
>> src/pkg/fmt/doc.go:167: Note: Fscan etc.  can read one character (rune)
>> past the input
>> On 2011/03/01 19:27:18, r wrote:
>>> one space
>>
>> ah... funnily enough the responsibility for that lies with some other
>> code of yours, also named fmt.
>
> my memory may be wrong but i believe td wrote fmt. all i added was retaining
indentation.

ah, i didn't look too closely and presumed you'd rewritten it
when you did that.

[r, 2011-03-02 18:15:07]

i like how this is going but don't understand the deletion of Width

http://codereview.appspot.com/4240056/diff/15001/src/pkg/fmt/scan.go
File src/pkg/fmt/scan.go (left):

http://codereview.appspot.com/4240056/diff/15001/src/pkg/fmt/scan.go#oldcode39
src/pkg/fmt/scan.go:39: Width() (wid int, ok bool)
why is this gone? the scan routine should be able to know how much input to
absorb. i must be missing something

http://codereview.appspot.com/4240056/diff/15001/src/pkg/fmt/scan.go
File src/pkg/fmt/scan.go (right):

http://codereview.appspot.com/4240056/diff/15001/src/pkg/fmt/scan.go#newcode43
src/pkg/fmt/scan.go:43: // recursively to Fscan etc. It should never be called -
an
Because ReadRune is implemented by the interface, Read will never be called by
the scanning routines and a valid implementation of ScanState may choose always
to return an error from Read.

http://codereview.appspot.com/4240056/diff/15001/src/pkg/fmt/scan.go#newcode153
src/pkg/fmt/scan.go:153: return 0, os.ErrorString("Read not implemented - use
ReadRune instead")
implemented is a peculiar word here, because it also means 'implement the
interface'. maybe
ScanState's Read should not be called. Use ReadRune

[r2, 2011-03-02 18:17:14]

And the whole one-space/two-space thing is a minefield anyway.  One space should
be necessary but for some reason everyone but me prefers to work in a colored,
fixed-width visual presentation model from the 1960s best suited to airline
reservation systems.

But I digress.

Back to your code.

-rob

[rog, 2011-03-02 18:30:43]

PTAL

http://codereview.appspot.com/4240056/diff/15001/src/pkg/fmt/scan.go
File src/pkg/fmt/scan.go (left):

http://codereview.appspot.com/4240056/diff/15001/src/pkg/fmt/scan.go#oldcode39
src/pkg/fmt/scan.go:39: Width() (wid int, ok bool)
On 2011/03/02 18:15:07, r wrote:
> why is this gone? the scan routine should be able to know how much input to
> absorb. i must be missing something

The scan routine can no longer absorb more than it is allowed, because ReadRune
returns EOF at the end of the allowed width. Thus the scan routine doesn't need
to care. AFAICS.

http://codereview.appspot.com/4240056/diff/15001/src/pkg/fmt/scan.go
File src/pkg/fmt/scan.go (right):

http://codereview.appspot.com/4240056/diff/15001/src/pkg/fmt/scan.go#newcode43
src/pkg/fmt/scan.go:43: // recursively to Fscan etc. It should never be called -
an
On 2011/03/02 18:15:07, r wrote:
> Because ReadRune is implemented by the interface, Read will never be called by
> the scanning routines and a valid implementation of ScanState may choose
always
> to return an error from Read.

Done.

http://codereview.appspot.com/4240056/diff/15001/src/pkg/fmt/scan.go#newcode153
src/pkg/fmt/scan.go:153: return 0, os.ErrorString("Read not implemented - use
ReadRune instead")
On 2011/03/02 18:15:07, r wrote:
> implemented is a peculiar word here, because it also means 'implement the
> interface'. maybe
> ScanState's Read should not be called. Use ReadRune

Done.

[r2, 2011-03-02 18:34:42]

the scanner may care anyway and knowing in advance how wide the field is could
be used for buffer management, etc. i suggest leaving it in but saying what you
wrote: you'll get EOF if you read past.

-rob

[r, 2011-03-02 18:35:48]

http://codereview.appspot.com/4240056/diff/16004/src/pkg/fmt/scan.go
File src/pkg/fmt/scan.go (right):

http://codereview.appspot.com/4240056/diff/16004/src/pkg/fmt/scan.go#newcode42
src/pkg/fmt/scan.go:42: // Because ReadRune is implemented by the interface,
Read will never be
maybe s/will/should/. it's a specification.

[rog, 2011-03-02 18:50:13]

> the scanner may care anyway and knowing in advance how wide the field is could
be used for buffer management, etc. i suggest leaving it in but saying what you
wrote: you'll get EOF if you read past.

done, but i think the documentation on ReadRune is sufficient in itself without
adding to the Width doc.

http://codereview.appspot.com/4240056/diff/16004/src/pkg/fmt/scan.go
File src/pkg/fmt/scan.go (right):

http://codereview.appspot.com/4240056/diff/16004/src/pkg/fmt/scan.go#newcode42
src/pkg/fmt/scan.go:42: // Because ReadRune is implemented by the interface,
Read will never be
On 2011/03/02 18:35:48, r wrote:
> maybe s/will/should/. it's a specification.

Done.

[r, 2011-03-02 18:52:09]

LGTM

thanks, it's good

[r, 2011-03-02 18:54:32]

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

fmt: allow recursive calls to Fscan etc.
Add a new Read method to ScanState so that it
satisfies the io.Reader interface; rename
Getrune and Ungetrune to ReadRune and UnreadRune.
Make sure ReadRune does not read past width restrictions;
remove now-unnecessary Width method from ScanState.
Also make the documentation a little clearer as to
how ReadRune and UnreadRune are used.

R=r, r2
CC=golang-dev
http://codereview.appspot.com/4240056

Committer: Rob Pike <r@golang.org>