code review 1912042: bufio: introduce Peek and PeekByte.

bufio: introduce Peek.

I'll leave whether or not this obsoletes UnreadByte for a future CL.

[nigeltao, 2010-08-02 08:10:01]

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

I'd like you to review this change.

[r, 2010-08-03 00:22:34]

we'll need PeekRune too

[rsc1, 2010-08-03 03:47:52]

a few design things

http://codereview.appspot.com/1912042/diff/2001/3001
File src/pkg/bufio/bufio.go (right):

http://codereview.appspot.com/1912042/diff/2001/3001#newcode119
src/pkg/bufio/bufio.go:119: m, err := io.ReadFull(b.rd, b.buf[b.w:b.r+n])
ReadFull isn't correct here.  If I Peek(1), Read(1), Peek(1), Read(1),
I won't get any actual buffering on the reads.  But see next comment.

http://codereview.appspot.com/1912042/diff/2001/3001#newcode126
src/pkg/bufio/bufio.go:126: func (b *Reader) Peek(p []byte) (int, os.Error) {
I was expecting Peek to return ([]byte, os.Error),
with the same caveat that ReadSlice has.
Then there's no copy.  I'd like to add a method
of the same signature, whatever it ends up being,
to bytes.Buffer, and it's only really useful if you
get the slice back.

I think it would look like

Peek(n) {
    for b.err != nil && b.w-b.r < n && b.w-b.r < len(b.buf) {
        b.fill()
    }
    m := b.w - b.r
    if m > n {
        m = n
    }
    err := b.err
    if m < n && err == nil {
        err = ErrBufferFull
    }
    return b.buf[b.r:b.r+m], err
}

(and then could delete ensure).

http://codereview.appspot.com/1912042/diff/2001/3001#newcode135
src/pkg/bufio/bufio.go:135: // PeekByte returns the i'th byte ahead without
advancing the reader.
With the Peek that returns a slice, this should be unnecessary.

[nigeltao_gnome, 2010-08-03 06:24:27]

On 3 August 2010 13:47,  <rsc@google.com> wrote:
> I think it would look like
>
> Peek(n) {
>    for b.err != nil && b.w-b.r < n && b.w-b.r < len(b.buf) {
>        b.fill()
>    }

Yes, your API is nicer. PTAL.

(The first != in your sketch should have been an ==).

[rsc1, 2010-08-03 06:51:50]

LGTM

leaving for r

http://codereview.appspot.com/1912042/diff/8001/7002
File src/pkg/bufio/bufio.go (right):

http://codereview.appspot.com/1912042/diff/8001/7002#newcode31
src/pkg/bufio/bufio.go:31: ErrNegativeIndex     os.Error = &Error{"bufio:
negative index"}
really negative count now

http://codereview.appspot.com/1912042/diff/8001/7002#newcode89
src/pkg/bufio/bufio.go:89: copy(b.buf[0:b.w-b.r], b.buf[b.r:b.w])
this call have to be so picky anymore (it predates copy)

copy(b.buf, b.buf[b.r:b.w])
b.w -= b.r

you could even drop the if b.w > b.r; copy will do the right thing.

http://codereview.appspot.com/1912042/diff/8001/7002#newcode106
src/pkg/bufio/bufio.go:106: // It fails with ErrBufferFull if n > the length of
b's buffer.
If Peek returns fewer than n bytes, it also returns an error explaining why the
read is short.  The error is ErrBufferFull if n is larger than b's buffer size.

http://codereview.appspot.com/1912042/diff/8001/7002#newcode159
src/pkg/bufio/bufio.go:159: copy(p[0:n], b.buf[b.r:b.r+n])
while we're on the topic
copy(p[0:n], b.buf[b.r:])

http://codereview.appspot.com/1912042/diff/8001/7002#newcode321
src/pkg/bufio/bufio.go:321: copy(buf[n:n+len(full[i])], full[i])
copy(buf[n:], full[i])

http://codereview.appspot.com/1912042/diff/8001/7002#newcode324
src/pkg/bufio/bufio.go:324: copy(buf[n:n+len(frag)], frag)
copy(buf[n:], frag)

[nigeltao, 2010-08-03 07:19:11]

http://codereview.appspot.com/1912042/diff/8001/7002
File src/pkg/bufio/bufio.go (right):

http://codereview.appspot.com/1912042/diff/8001/7002#newcode31
src/pkg/bufio/bufio.go:31: ErrNegativeIndex     os.Error = &Error{"bufio:
negative index"}
On 2010/08/03 06:51:50, rsc1 wrote:
> really negative count now
> 

Done.

http://codereview.appspot.com/1912042/diff/8001/7002#newcode89
src/pkg/bufio/bufio.go:89: copy(b.buf[0:b.w-b.r], b.buf[b.r:b.w])
On 2010/08/03 06:51:50, rsc1 wrote:
> this call have to be so picky anymore (it predates copy)
> 
> copy(b.buf, b.buf[b.r:b.w])
> b.w -= b.r
> 
> you could even drop the if b.w > b.r; copy will do the right thing.
> 

Done.

http://codereview.appspot.com/1912042/diff/8001/7002#newcode106
src/pkg/bufio/bufio.go:106: // It fails with ErrBufferFull if n > the length of
b's buffer.
On 2010/08/03 06:51:50, rsc1 wrote:
> If Peek returns fewer than n bytes, it also returns an error explaining why
the
> read is short.  The error is ErrBufferFull if n is larger than b's buffer
size.
> 

Done.

http://codereview.appspot.com/1912042/diff/8001/7002#newcode159
src/pkg/bufio/bufio.go:159: copy(p[0:n], b.buf[b.r:b.r+n])
On 2010/08/03 06:51:50, rsc1 wrote:
> while we're on the topic
> copy(p[0:n], b.buf[b.r:])
> 

Done.

http://codereview.appspot.com/1912042/diff/8001/7002#newcode321
src/pkg/bufio/bufio.go:321: copy(buf[n:n+len(full[i])], full[i])
On 2010/08/03 06:51:50, rsc1 wrote:
> copy(buf[n:], full[i])
> 

Done.

http://codereview.appspot.com/1912042/diff/8001/7002#newcode324
src/pkg/bufio/bufio.go:324: copy(buf[n:n+len(frag)], frag)
On 2010/08/03 06:51:50, rsc1 wrote:
> copy(buf[n:], frag)
> 

Done.

[r, 2010-08-03 07:45:01]

LGTM

[rsc1, 2010-08-03 19:34:17]

LGTM

[r2, 2010-08-03 19:45:32]

LGTM but update the CL description: there is no PeekByte

[r2, 2010-08-03 19:45:53]

never mind, i see it's already been updated.

[nigeltao, 2010-08-03 23:44:09]

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

bufio: introduce Peek.

I'll leave whether or not this obsoletes UnreadByte for a future CL.

R=r, rsc
CC=golang-dev
http://codereview.appspot.com/1912042