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

bytes: add Buffer.UnreadRune, Buffer.UnreadByte

[rog, 2010-09-23 11:01:42]

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

I'd like you to review this change.

[rog, 2010-09-23 11:02:26]

BTW, is there a reason that bytes.Buffer does not implement Seek?

On 23 September 2010 12:01,  <rogpeppe@gmail.com> wrote:
> Reviewers: r,
>
> Message:
> Hello r (cc: golang-dev@googlegroups.com, rsc),
>
> I'd like you to review this change.
>
>
> Description:
> bytes: add Buffer.UnreadRune
>
> Please review this at http://codereview.appspot.com/2194045/
>
> Affected files:
>  M src/pkg/bytes/buffer.go
>  M src/pkg/bytes/buffer_test.go
>
>
> Index: src/pkg/bytes/buffer.go
> ===================================================================
> --- a/src/pkg/bytes/buffer.go
> +++ b/src/pkg/bytes/buffer.go
> @@ -249,6 +249,17 @@
>        return r, n, nil
>  }
>
> +// UnreadRune makes the previous rune available
> +// to be read again.
> +func (b *Buffer) UnreadRune() os.Error {
> +       if b.off <= 0 {
> +               return os.NewError("UnreadRune on empty buffer")
> +       }
> +       _, n := utf8.DecodeLastRune(b.buf[0:b.off])
> +       b.off -= n
> +       return nil
> +}
> +
>  // NewBuffer creates and initializes a new Buffer using buf as its initial
>  // contents.  It is intended to prepare a Buffer to read existing data.  It
>  // can also be used to to size the internal buffer for writing.  To do
> that,
> Index: src/pkg/bytes/buffer_test.go
> ===================================================================
> --- a/src/pkg/bytes/buffer_test.go
> +++ b/src/pkg/bytes/buffer_test.go
> @@ -289,14 +289,29 @@
>                t.Fatalf("incorrect result from WriteRune: %q not %q",
> buf.Bytes(), b)
>        }
>
> +       p := make([]byte, utf8.UTFMax)
>        // Read it back with ReadRune
>        for r := 0; r < NRune; r++ {
> -               size := utf8.EncodeRune(r, b)
> +               size := utf8.EncodeRune(r, p)
>                nr, nbytes, err := buf.ReadRune()
>                if nr != r || nbytes != size || err != nil {
>                        t.Fatalf("ReadRune(0x%x) got 0x%x,%d not 0x%x,%d
> (err=%s)", r, nr, nbytes, r, size, err)
>                }
>        }
> +
> +       // Check that UnreadRune works
> +       buf.Reset()
> +       buf.Write(b)
> +       for r := 0; r < NRune; r++ {
> +               r1, size, _ := buf.ReadRune()
> +               if err := buf.UnreadRune(); err != nil {
> +                       t.Fatalf("UnreadRune(%x0x) got error %q", r, err)
> +               }
> +               r2, nbytes, err := buf.ReadRune()
> +               if r1 != r2 || r1 != r || nbytes != size || err != nil {
> +                       t.Fatalf("ReadRune(0x%x) after UnreadRune got
> 0x%x,%d not 0x%x,%d (err=%s)", r, r2, nbytes, r, size, err)
> +               }
> +       }
>  }
>
>
>
>
>

[rog, 2010-09-23 11:03:12]

scratch that, it's obvious really.

On 23 September 2010 12:02, roger peppe <rogpeppe@gmail.com> wrote:
> BTW, is there a reason that bytes.Buffer does not implement Seek?
>
> On 23 September 2010 12:01,  <rogpeppe@gmail.com> wrote:
>> Reviewers: r,
>>
>> Message:
>> Hello r (cc: golang-dev@googlegroups.com, rsc),
>>
>> I'd like you to review this change.
>>
>>
>> Description:
>> bytes: add Buffer.UnreadRune
>>
>> Please review this at http://codereview.appspot.com/2194045/
>>
>> Affected files:
>>  M src/pkg/bytes/buffer.go
>>  M src/pkg/bytes/buffer_test.go
>>
>>
>> Index: src/pkg/bytes/buffer.go
>> ===================================================================
>> --- a/src/pkg/bytes/buffer.go
>> +++ b/src/pkg/bytes/buffer.go
>> @@ -249,6 +249,17 @@
>>        return r, n, nil
>>  }
>>
>> +// UnreadRune makes the previous rune available
>> +// to be read again.
>> +func (b *Buffer) UnreadRune() os.Error {
>> +       if b.off <= 0 {
>> +               return os.NewError("UnreadRune on empty buffer")
>> +       }
>> +       _, n := utf8.DecodeLastRune(b.buf[0:b.off])
>> +       b.off -= n
>> +       return nil
>> +}
>> +
>>  // NewBuffer creates and initializes a new Buffer using buf as its initial
>>  // contents.  It is intended to prepare a Buffer to read existing data.  It
>>  // can also be used to to size the internal buffer for writing.  To do
>> that,
>> Index: src/pkg/bytes/buffer_test.go
>> ===================================================================
>> --- a/src/pkg/bytes/buffer_test.go
>> +++ b/src/pkg/bytes/buffer_test.go
>> @@ -289,14 +289,29 @@
>>                t.Fatalf("incorrect result from WriteRune: %q not %q",
>> buf.Bytes(), b)
>>        }
>>
>> +       p := make([]byte, utf8.UTFMax)
>>        // Read it back with ReadRune
>>        for r := 0; r < NRune; r++ {
>> -               size := utf8.EncodeRune(r, b)
>> +               size := utf8.EncodeRune(r, p)
>>                nr, nbytes, err := buf.ReadRune()
>>                if nr != r || nbytes != size || err != nil {
>>                        t.Fatalf("ReadRune(0x%x) got 0x%x,%d not 0x%x,%d
>> (err=%s)", r, nr, nbytes, r, size, err)
>>                }
>>        }
>> +
>> +       // Check that UnreadRune works
>> +       buf.Reset()
>> +       buf.Write(b)
>> +       for r := 0; r < NRune; r++ {
>> +               r1, size, _ := buf.ReadRune()
>> +               if err := buf.UnreadRune(); err != nil {
>> +                       t.Fatalf("UnreadRune(%x0x) got error %q", r, err)
>> +               }
>> +               r2, nbytes, err := buf.ReadRune()
>> +               if r1 != r2 || r1 != r || nbytes != size || err != nil {
>> +                       t.Fatalf("ReadRune(0x%x) after UnreadRune got
>> 0x%x,%d not 0x%x,%d (err=%s)", r, r2, nbytes, r, size, err)
>> +               }
>> +       }
>>  }
>>
>>
>>
>>
>>
>

[r, 2010-09-23 12:41:45]

I'm not sure about this.

First, if we have unreadrune there should first be unreadbyte.

Second, I'm not convinced yet that the invariants in the implementation allow
such a simple algorithm for unreadrune to be reliable.  In fact, I'm pretty sure
they're not, because

Third, you need at least to explain in the comment when it's OK to call this and
what to expect.  Can I call it twice in a row? Sometimes...

In bufio I did a careful job guaranteeing that you can unread one rune if the
previous operation was a readrune.  I like that implementation, but I'm not sure
that bytes.Buffer needs to be so careful; I'm also not sure it doesn't.  Byte
buffers are everywhere and doing it this way adds cost.

In short, I'm not against unreadrune, I'm just against such a simple approach,
and I'm against too much overhead to support it.

This package is very important and every detail must be right.

[rog, 2010-09-23 13:16:20]

On 23 September 2010 13:41,  <r@golang.org> wrote:
> I'm not sure about this.
>
> First, if we have unreadrune there should first be unreadbyte.

true.

> Second, I'm not convinced yet that the invariants in the implementation
> allow such a simple algorithm for unreadrune to be reliable.  In fact,
> I'm pretty sure they're not, because

because...?
i think the invariants in the implementation allow the simple
algorithm to work when the last operation was a ReadRune.
would it be enough to document the return result as undefined otherwise?

> In bufio I did a careful job guaranteeing that you can unread one rune
> if the previous operation was a readrune.  I like that implementation,
> but I'm not sure that bytes.Buffer needs to be so careful; I'm also not
> sure it doesn't.  Byte buffers are everywhere and doing it this way adds
> cost.

there's cost to the UnreadRune, but is it less cost than wrapping the
Buffer in a bufio.Reader?

> In short, I'm not against unreadrune, I'm just against such a simple
> approach, and I'm against too much overhead to support it.

yes, that's why i didn't add an extra field, bufio style,
so there's no overhead when UnreadRune is not used.

[rog, 2010-09-23 14:05:59]

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

Please take another look.

[rsc, 2010-09-23 14:34:43]

The invariant here is that after Read has caught up with Write,
the space can be reused.  Backing up in general is incompatible
with that.  As long as the last method call on the Buffer was
ReadRune, then UnreadRune is safe.  But it's not safe if there
was a ReadRune and then a Read, or a ReadRune and then a Write,
even if that Read/Write processed 0 bytes.

I think these would be okay to implement:

// UnreadRune unreads the last rune returned by ReadRune.
// If the most recent read or write operation on the buffer was
// not a ReadRune, UnreadRune returns an error.  (In this regard
// it is stricter than UnreadByte, which will unread the last byte
// from any read operation.)

// UnreadByte unreads the last byte returned by the most recent
// read operation.  If the last read operation returned zero bytes
// or if a write has happened since the last read, UnreadByte
// returns an error.

You'd need to maintain a single 3-state field lastRead in the
Buffer that could take the values opRead, opReadRune, opInvalid.

Russ

[rog, 2010-09-23 16:15:49]

PTAL

On 23 September 2010 15:34, Russ Cox <rsc@golang.org> wrote:
> // UnreadByte unreads the last byte returned by the most recent
> // read operation.  If the last read operation returned zero bytes
> // or if a write has happened since the last read, UnreadByte
> // returns an error.

BTW, i don't see why UnreadByte should return an error
if the last read returned zero bytes. the next read will return
zero bytes too - it will "undo" the last operation just fine.

bufio doesn't forbid UnreadRune after ReadRune has returned eof.

[gri, 2010-09-23 16:50:01]

For what it's worth:

I am not comfortable with this CL because it requires that the common operations
(Read, ReadByte, Write, etc.) and the buffer state all become more complex
because of a couple of rarely used functions. That is, the sheer presence of
these functions, even if not used, makes the core operations more expensive. I
admit that the expense is tiny, but the extra complexity is still present. Also,
the unreads are only correctly possible in certain situations. Furthermore, it
opens the door to other stuff that doesn't necessarily need to be in this
package. I'd put maximum efficiency of bytes.Buffer before any convenience that
is rarely needed.

Unread functions are typically used when scanning some input and one needs some
form of look-ahead that may need to be undone. In my experience, these
situations almost always can be written equally well and sometimes more
efficiently by maintaining the lookahead in the respective scanners and by never
"unreading" something that has already been read. Often, it also leads to a
cleaner invariant to maintain.

If one really wants the convenience of an unread, why not have a wrapper object
that takes a bytes.Buffer (or some general io.Reader) and provides ReadByte,
UnreadByte, ReadRune, UnreadRune, etc.? Than the price is only paid if the
functionality is needed.

I'll leave it to r and rsc to decide what to do with this.
- gri

[rsc, 2010-09-23 16:52:17]

> BTW, i don't see why UnreadByte should return an error
> if the last read returned zero bytes. the next read will return
> zero bytes too - it will "undo" the last operation just fine.

it's call Unread*Byte* not UnreadByteOrNopIfReadByteGotEOF.
it returns an error if it cannot unread a byte.

> bufio doesn't forbid UnreadRune after ReadRune has returned eof.

that's a bug: ReadRune should set lastRuneSize = -1
at the beginning of the function.  the call to b.fill()
might flush the buffer, making the adjustment of b.r
by UnreadRune unsafe.

russ

[rog, 2010-09-23 17:03:26]

On 23 September 2010 17:52, Russ Cox <rsc@golang.org> wrote:
>> BTW, i don't see why UnreadByte should return an error
>> if the last read returned zero bytes. the next read will return
>> zero bytes too - it will "undo" the last operation just fine.
>
> it's call Unread*Byte* not UnreadByteOrNopIfReadByteGotEOF.
> it returns an error if it cannot unread a byte.

the way i looked at it, ReadByte followed by UnreadByte is
always a nop. but i guess it doesn't make much difference.
hardly anyone's likely to check the return value of UnreadByte/Rune
anyway, i'd guess.

i'll change it anyway.

>> bufio doesn't forbid UnreadRune after ReadRune has returned eof.
>
> that's a bug: ReadRune should set lastRuneSize = -1
> at the beginning of the function.  the call to b.fill()
> might flush the buffer, making the adjustment of b.r
> by UnreadRune unsafe.

[rog, 2010-09-23 17:18:41]

On 23 September 2010 17:50,  <gri@golang.org> wrote:
> For what it's worth:
>
> I am not comfortable with this CL because it requires that the common
> operations (Read, ReadByte, Write, etc.) and the buffer state all become
> more complex because of a couple of rarely used functions.

that's why my initial implementation left most of the code unchanged,
and just invoked undefined behaviour if the unread functions were
called inappropriately. no runtime cost. AFAICS the "undefined
behaviour" in question might be to return EOF inappropriately,
which isn't a great hazard.

> If one really wants the convenience of an unread, why not have a wrapper
> object that takes a bytes.Buffer (or some general io.Reader) and
> provides ReadByte, UnreadByte, ReadRune, UnreadRune, etc.? Than the
> price is only paid if the functionality is needed.

that wrapper is called bufio, but there's a reasonably stiff price
for using it. any such wrapper will incur a significant
cost if the reader is doing many small read ops on a bytes.Buffer (which could
potentially be inlined in the future, but not when using a wrapper
interface)

i made this CL because i thought there was a simple and almost
cost-free way of making fmt.Scan work correctly on bytes.Buffer.

if the consensus is that this isn't worth it, then i'll abandon.

[rsc1, 2010-09-23 17:25:28]

I think being able to fmt.Scan from a bytes.Buffer 
instead of having to wrap it in a bufio.Reader is
worth 1 extra statement at the beginning of the
methods (lastRead = opInvalid).

In the code, have to be careful to start with lastRead invalid
and only change to read if the function succeeds.

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

http://codereview.appspot.com/2194045/diff/14001/src/pkg/bytes/buffer.go#newc...
src/pkg/bytes/buffer.go:207: b.lastRead = opRead
b.lastRead = opInvalid

http://codereview.appspot.com/2194045/diff/14001/src/pkg/bytes/buffer.go#newc...
src/pkg/bytes/buffer.go:211: b.lastRead = opRead
d

http://codereview.appspot.com/2194045/diff/14001/src/pkg/bytes/buffer.go#newc...
src/pkg/bytes/buffer.go:215: b.off += n
if n > 0 {
    b.lastRead = opRead
}

http://codereview.appspot.com/2194045/diff/14001/src/pkg/bytes/buffer.go#newc...
src/pkg/bytes/buffer.go:224: b.lastRead = opRead
b.lastRead = opInvalid

http://codereview.appspot.com/2194045/diff/14001/src/pkg/bytes/buffer.go#newc...
src/pkg/bytes/buffer.go:230: b.off += n
if n > 0 {
    b.lastRead = opRead
}

http://codereview.appspot.com/2194045/diff/14001/src/pkg/bytes/buffer.go#newc...
src/pkg/bytes/buffer.go:237: b.lastRead = opRead
opInvalid

http://codereview.appspot.com/2194045/diff/14001/src/pkg/bytes/buffer.go#newc...
src/pkg/bytes/buffer.go:241: b.lastRead = opRead
d

http://codereview.appspot.com/2194045/diff/14001/src/pkg/bytes/buffer.go#newc...
src/pkg/bytes/buffer.go:246: return c, nil
b.lastRead = opRead

http://codereview.appspot.com/2194045/diff/14001/src/pkg/bytes/buffer.go#newc...
src/pkg/bytes/buffer.go:255: b.lastRead = opReadRune
opInvalid

http://codereview.appspot.com/2194045/diff/14001/src/pkg/bytes/buffer.go#newc...
src/pkg/bytes/buffer.go:259: b.lastRead = opReadRune
d

http://codereview.appspot.com/2194045/diff/14001/src/pkg/bytes/buffer.go#newc...
src/pkg/bytes/buffer.go:262: c := b.buf[b.off]
b.lastRead = opReadRune

[gri, 2010-09-23 17:43:03]

On Thu, Sep 23, 2010 at 10:18 AM, roger peppe <rogpeppe@gmail.com> wrote:

> On 23 September 2010 17:50,  <gri@golang.org> wrote:
> > For what it's worth:
> >
> > I am not comfortable with this CL because it requires that the common
> > operations (Read, ReadByte, Write, etc.) and the buffer state all become
> > more complex because of a couple of rarely used functions.
>
> that's why my initial implementation left most of the code unchanged,
> and just invoked undefined behaviour if the unread functions were
> called inappropriately. no runtime cost. AFAICS the "undefined
> behaviour" in question might be to return EOF inappropriately,
> which isn't a great hazard.
>
> > If one really wants the convenience of an unread, why not have a wrapper
> > object that takes a bytes.Buffer (or some general io.Reader) and
> > provides ReadByte, UnreadByte, ReadRune, UnreadRune, etc.? Than the
> > price is only paid if the functionality is needed.
>
> that wrapper is called bufio, but there's a reasonably stiff price
> for using it. any such wrapper will incur a significant
> cost if the reader is doing many small read ops on a bytes.Buffer (which
> could
> potentially be inlined in the future, but not when using a wrapper
> interface)
>

I wouldn't use bufio. UnreadRune can only be used if ReadRune was called
before, and I suspect that UnreadByte is only called if ReadByte was called
before. Thus, we are talking about sequential reading of individual
characters. I would have a very light-weight specific wrapper just for that
if bufio is too expensive to use. That wrapper would only accept an
io.Buffer. That way you pay as you go.

But if utmost speed is not important, bufio should do fine. In fact, if we
are reading sequentially, it should not be much more expensive (do you have
evidence to the contrary?). As it is, we are starting to blur the boundaries
between bytes.Buffer and bufio.

- gri



>
> i made this CL because i thought there was a simple and almost
> cost-free way of making fmt.Scan work correctly on bytes.Buffer.
>
> if the consensus is that this isn't worth it, then i'll abandon.
>

[rog, 2010-09-23 18:18:50]

On 23 September 2010 18:42, Robert Griesemer <gri@golang.org> wrote:
> I wouldn't use bufio. UnreadRune can only be used if ReadRune was called
> before, and I suspect that UnreadByte is only called if ReadByte was called
> before. Thus, we are talking about sequential reading of individual
> characters. I would have a very light-weight specific wrapper just for that
> if bufio is too expensive to use. That wrapper would only accept an
> io.Buffer. That way you pay as you go.
> But if utmost speed is not important, bufio should do fine. In fact, if we
> are reading sequentially, it should not be much more expensive (do you have
> evidence to the contrary?). As it is, we are starting to blur the boundaries
> between bytes.Buffer and bufio.

the boundaries are already blurred. such is the nature of the Go interface :-)

a quick test shows that using bytes.Buffer is about 15% faster than
using layering bufio.Reader on top of it, but that ratio will go up
when (if?) the compiler starts to inline function calls.

the overhead of the extra state management doesn't impact performance
in the slightest (my tests came out slightly faster with the state
management *in*...).

i think that possibly the strongest argument for this change is that
bytes.Buffer is very often used, and it would be nice if fmt.Scan
"just worked" without needing to import an additional module.
i'm sure it will save a few puzzled requests to golang-nuts in the future.

especially as the cost really isn't that great.

[rog, 2010-09-23 18:25:35]

PTAL.

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

http://codereview.appspot.com/2194045/diff/14001/src/pkg/bytes/buffer.go#newc...
src/pkg/bytes/buffer.go:207: b.lastRead = opRead
On 2010/09/23 17:25:28, rsc1 wrote:
> b.lastRead = opInvalid
> 

Done.

http://codereview.appspot.com/2194045/diff/14001/src/pkg/bytes/buffer.go#newc...
src/pkg/bytes/buffer.go:211: b.lastRead = opRead
On 2010/09/23 17:25:28, rsc1 wrote:
> d
> 

Done.

http://codereview.appspot.com/2194045/diff/14001/src/pkg/bytes/buffer.go#newc...
src/pkg/bytes/buffer.go:215: b.off += n
On 2010/09/23 17:25:28, rsc1 wrote:
> if n > 0 {
>     b.lastRead = opRead
> }
> 

Done.

http://codereview.appspot.com/2194045/diff/14001/src/pkg/bytes/buffer.go#newc...
src/pkg/bytes/buffer.go:224: b.lastRead = opRead
On 2010/09/23 17:25:28, rsc1 wrote:
> b.lastRead = opInvalid
> 

Done.

http://codereview.appspot.com/2194045/diff/14001/src/pkg/bytes/buffer.go#newc...
src/pkg/bytes/buffer.go:230: b.off += n
On 2010/09/23 17:25:28, rsc1 wrote:
> if n > 0 {
>     b.lastRead = opRead
> }
> 

Done.

http://codereview.appspot.com/2194045/diff/14001/src/pkg/bytes/buffer.go#newc...
src/pkg/bytes/buffer.go:237: b.lastRead = opRead
On 2010/09/23 17:25:28, rsc1 wrote:
> opInvalid
> 

Done.

http://codereview.appspot.com/2194045/diff/14001/src/pkg/bytes/buffer.go#newc...
src/pkg/bytes/buffer.go:241: b.lastRead = opRead
On 2010/09/23 17:25:28, rsc1 wrote:
> d

Done.

http://codereview.appspot.com/2194045/diff/14001/src/pkg/bytes/buffer.go#newc...
src/pkg/bytes/buffer.go:246: return c, nil
On 2010/09/23 17:25:28, rsc1 wrote:
> b.lastRead = opRead
> 

Done.

http://codereview.appspot.com/2194045/diff/14001/src/pkg/bytes/buffer.go#newc...
src/pkg/bytes/buffer.go:255: b.lastRead = opReadRune
On 2010/09/23 17:25:28, rsc1 wrote:
> opInvalid
> 

Done.

http://codereview.appspot.com/2194045/diff/14001/src/pkg/bytes/buffer.go#newc...
src/pkg/bytes/buffer.go:259: b.lastRead = opReadRune
On 2010/09/23 17:25:28, rsc1 wrote:
> d
> 

Done.

http://codereview.appspot.com/2194045/diff/14001/src/pkg/bytes/buffer.go#newc...
src/pkg/bytes/buffer.go:262: c := b.buf[b.off]
On 2010/09/23 17:25:28, rsc1 wrote:
> b.lastRead = opReadRune
> 

Done.

[r, 2010-09-23 20:55:00]

I haven't looked again at this CL, but I was thinking that you could
solve the problem with no code changes, provided you were willing to
have different semantics from bufio when the user erred, if you just
document it right.  Use something like that in the original CL, but
make the comment say it's only guaranteed to work if you call it
immediately after a ReadRune (and make sure the upside of the
guarantee holds).  This makes it work correctly if you use it
correctly, and maybe or maybe not if you use it incorrectly.  It's
like bufio but without the checks.

This leaves the rest of the code untouched, which I think is good. I
agree with gri that I'm nervous to put code in the critical path for
rarely used functions.  (I'm not as bothered by bufio because a) it's
used less and b) its purpose is to take care of things like this.)

-rob

[rog, 2010-09-27 09:13:01]

On 23 September 2010 21:54, Rob 'Commander' Pike <r@golang.org> wrote:
> I haven't looked again at this CL, but I was thinking that you could
> solve the problem with no code changes, provided you were willing to
> have different semantics from bufio when the user erred, if you just
> document it right.  Use something like that in the original CL, but
> make the comment say it's only guaranteed to work if you call it
> immediately after a ReadRune (and make sure the upside of the
> guarantee holds).  This makes it work correctly if you use it
> correctly, and maybe or maybe not if you use it incorrectly.  It's
> like bufio but without the checks.

that was my original plan, but perhaps you could suggest a form
of words for the comments? mine were evidently not good enough.
here's the version that most closely corresponds to what
you're suggesting.

http://codereview.appspot.com/2194045/diff/9001/src/pkg/bytes/buffer.go

[rsc, 2010-09-27 14:22:20]

Saying something is undefined without
enforcement scares me.
The lazy implementation makes UnreadRune
and UnreadByte work almost all the time,
except when they don't (basically, when
the last read caught up with the writer
and then there was a write).  People will
come to depend on this behavior to work
and it will constrain future implementations,
for example ones that might want to reuse
the buffer more aggressively.

I believe we should use the implementation at
http://codereview.appspot.com/2194045/diff/23002/src/pkg/bytes/buffer.go
The "overhead" here is one instruction -
a single memory write - per Read or Write.
The functions are complex enough already
that I can't see it making a performance
difference.  It's probably 10-50x faster than
wrapping the bytes.Buffer in a bufio.Reader
and simpler than explaining to users why
you have to buffer a buffer just to call
fmt.Scanf.

Russ

[rog, 2010-09-27 15:20:10]

i'll let you sort it out between yourselves :-)

On 27 September 2010 15:22, Russ Cox <rsc@golang.org> wrote:
> Saying something is undefined without
> enforcement scares me.
> The lazy implementation makes UnreadRune
> and UnreadByte work almost all the time,
> except when they don't (basically, when
> the last read caught up with the writer
> and then there was a write).  People will
> come to depend on this behavior to work
> and it will constrain future implementations,
> for example ones that might want to reuse
> the buffer more aggressively.
>
> I believe we should use the implementation at
> http://codereview.appspot.com/2194045/diff/23002/src/pkg/bytes/buffer.go
> The "overhead" here is one instruction -
> a single memory write - per Read or Write.
> The functions are complex enough already
> that I can't see it making a performance
> difference.  It's probably 10-50x faster than
> wrapping the bytes.Buffer in a bufio.Reader
> and simpler than explaining to users why
> you have to buffer a buffer just to call
> fmt.Scanf.
>
> Russ
>

[r2, 2010-09-28 18:30:02]

i agree with russ

-rob

[rsc1, 2010-11-01 20:44:40]

LGTM

please sync and re-hg mail

[rog, 2010-12-02 19:24:12]

*** Abandoned ***