code review 3421041: bytes: add Buffer.UnreadRune, Buffer.UnreadByte

bytes: add Buffer.UnreadRune, Buffer.UnreadByte

[rog, 2010-12-02 19:46:32]

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

I'd like you to review this change.

[rog, 2010-12-02 19:47:42]

accidentally deleted previous version of this issue.
( it was http://codereview.appspot.com/2194045/ )

On 2 December 2010 19:46,  <rogpeppe@gmail.com> wrote:
> Reviewers: rsc,
>
> Message:
> Hello rsc (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change.
>
>
> Description:
> bytes: add Buffer.UnreadRune, Buffer.UnreadByte
>
> Please review this at http://codereview.appspot.com/3421041/
>
> Affected files:
>  M src/pkg/bytes/buffer.go
>  M src/pkg/bytes/buffer_test.go
>
>
>

[r, 2010-12-02 20:32:00]

http://codereview.appspot.com/3421041/diff/2001/src/pkg/bytes/buffer.go
File src/pkg/bytes/buffer.go (right):

http://codereview.appspot.com/3421041/diff/2001/src/pkg/bytes/buffer.go#newco...
src/pkg/bytes/buffer.go:22: lastRead  int
no comment??
possibly even worth making this a type other than int

http://codereview.appspot.com/3421041/diff/2001/src/pkg/bytes/buffer.go#newco...
src/pkg/bytes/buffer.go:25: const (
need a comment describing what these are

http://codereview.appspot.com/3421041/diff/2001/src/pkg/bytes/buffer.go#newco...
src/pkg/bytes/buffer.go:275: return os.NewError("previous operation was not
ReadRune")
error should identify itself:
bytes.Buffer: ...

http://codereview.appspot.com/3421041/diff/2001/src/pkg/bytes/buffer.go#newco...
src/pkg/bytes/buffer.go:290: return os.NewError("previous operation was not a
read")
ditto

[rog, 2010-12-03 09:27:29]

PTAL

http://codereview.appspot.com/3421041/diff/2001/src/pkg/bytes/buffer.go
File src/pkg/bytes/buffer.go (right):

http://codereview.appspot.com/3421041/diff/2001/src/pkg/bytes/buffer.go#newco...
src/pkg/bytes/buffer.go:22: lastRead  int
On 2010/12/02 20:32:00, r wrote:
> no comment??
> possibly even worth making this a type other than int

Done.

http://codereview.appspot.com/3421041/diff/2001/src/pkg/bytes/buffer.go#newco...
src/pkg/bytes/buffer.go:25: const (
On 2010/12/02 20:32:00, r wrote:
> need a comment describing what these are

Done.

http://codereview.appspot.com/3421041/diff/2001/src/pkg/bytes/buffer.go#newco...
src/pkg/bytes/buffer.go:275: return os.NewError("previous operation was not
ReadRune")
On 2010/12/02 20:32:00, r wrote:
> error should identify itself:
> bytes.Buffer: ...

Done.

http://codereview.appspot.com/3421041/diff/2001/src/pkg/bytes/buffer.go#newco...
src/pkg/bytes/buffer.go:290: return os.NewError("previous operation was not a
read")
On 2010/12/02 20:32:00, r wrote:
> ditto

Done.

[r, 2010-12-03 20:15:31]

http://codereview.appspot.com/3421041/diff/9001/src/pkg/bytes/buffer.go
File src/pkg/bytes/buffer.go (right):

http://codereview.appspot.com/3421041/diff/9001/src/pkg/bytes/buffer.go#newco...
src/pkg/bytes/buffer.go:31: opInvalid  = readOp(iota) // Non-read operation.
i'd prefer
opInvalid readOp = iota

http://codereview.appspot.com/3421041/diff/9001/src/pkg/bytes/buffer.go#newco...
src/pkg/bytes/buffer.go:295: return os.ErrorString("bytes.Buffer: previous
operation was not a read")
my fault but i think you need to say
bytes.Buffer: UnreadByte: ...
and ditto for UnreadRune above
(think of what the message would mean printed in some log somewhere.)

http://codereview.appspot.com/3421041/diff/9001/src/pkg/bytes/buffer_test.go
File src/pkg/bytes/buffer_test.go (right):

http://codereview.appspot.com/3421041/diff/9001/src/pkg/bytes/buffer_test.go#...
src/pkg/bytes/buffer_test.go:308: t.Fatalf("UnreadRune(%x0x) got error %q", r,
err)
bizarre format; see below

http://codereview.appspot.com/3421041/diff/9001/src/pkg/bytes/buffer_test.go#...
src/pkg/bytes/buffer_test.go:312: t.Fatalf("ReadRune(0x%x) after UnreadRune got
0x%x,%d not 0x%x,%d (err=%s)", r, r2, nbytes, r, size, err)
might as well use U+%.4x here and above

[rog, 2010-12-06 14:37:43]

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

Please take another look.

[r, 2010-12-06 16:14:51]

one tiny tweak. looks good.

http://codereview.appspot.com/3421041/diff/22001/src/pkg/bytes/buffer.go
File src/pkg/bytes/buffer.go (right):

http://codereview.appspot.com/3421041/diff/22001/src/pkg/bytes/buffer.go#newc...
src/pkg/bytes/buffer.go:33: opRead                   // Any other read operation
s/$/./ or delete the others, for consistency

[rog, 2010-12-06 17:37:43]

PTAL

http://codereview.appspot.com/3421041/diff/9001/src/pkg/bytes/buffer.go
File src/pkg/bytes/buffer.go (right):

http://codereview.appspot.com/3421041/diff/9001/src/pkg/bytes/buffer.go#newco...
src/pkg/bytes/buffer.go:31: opInvalid  = readOp(iota) // Non-read operation.
On 2010/12/03 20:15:31, r wrote:
> i'd prefer
> opInvalid readOp = iota

Done.

http://codereview.appspot.com/3421041/diff/9001/src/pkg/bytes/buffer.go#newco...
src/pkg/bytes/buffer.go:295: return os.ErrorString("bytes.Buffer: previous
operation was not a read")
On 2010/12/03 20:15:31, r wrote:
> my fault but i think you need to say
> bytes.Buffer: UnreadByte: ...
> and ditto for UnreadRune above
> (think of what the message would mean printed in some log somewhere.)

Done.

http://codereview.appspot.com/3421041/diff/9001/src/pkg/bytes/buffer_test.go
File src/pkg/bytes/buffer_test.go (right):

http://codereview.appspot.com/3421041/diff/9001/src/pkg/bytes/buffer_test.go#...
src/pkg/bytes/buffer_test.go:308: t.Fatalf("UnreadRune(%x0x) got error %q", r,
err)
On 2010/12/03 20:15:31, r wrote:
> bizarre format; see below

Done.

http://codereview.appspot.com/3421041/diff/9001/src/pkg/bytes/buffer_test.go#...
src/pkg/bytes/buffer_test.go:312: t.Fatalf("ReadRune(0x%x) after UnreadRune got
0x%x,%d not 0x%x,%d (err=%s)", r, r2, nbytes, r, size, err)
On 2010/12/03 20:15:31, r wrote:
> might as well use U+%.4x here and above

Done.

http://codereview.appspot.com/3421041/diff/22001/src/pkg/bytes/buffer.go
File src/pkg/bytes/buffer.go (right):

http://codereview.appspot.com/3421041/diff/22001/src/pkg/bytes/buffer.go#newc...
src/pkg/bytes/buffer.go:33: opRead                   // Any other read operation
On 2010/12/06 16:14:51, r wrote:
> s/$/./ or delete the others, for consistency

Done.

[r, 2010-12-06 19:05:22]

LGTM

[r, 2010-12-06 19:10:16]

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

bytes: add Buffer.UnreadRune, Buffer.UnreadByte

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

Committer: Rob Pike <r@golang.org>