code review 6565056: bufio: Implement io.ReaderFrom for (*Writer).

bufio: Implement io.ReaderFrom for (*Writer).

This is part 2 of 2 for issue 4028.

benchmark                        old ns/op    new ns/op    delta
BenchmarkWriterCopyOptimal           53293        28326  -46.85%
BenchmarkWriterCopyUnoptimal         53757        30537  -43.19%
BenchmarkWriterCopyNoReadFrom        53192        36642  -31.11%

Fixes issue 4028.

[chaten, 2012-09-27 09:27:00]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/

[nigeltao, 2012-09-28 04:49:24]

https://codereview.appspot.com/6565056/diff/5001/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

https://codereview.appspot.com/6565056/diff/5001/src/pkg/bufio/bufio.go#newco...
src/pkg/bufio/bufio.go:582: for m, err = b.readBuf(r); m > 0 && err == nil; m,
err = b.readBuf(r) {
This isn't right. "godoc io Reader" says that it's valid for r.Read to return m
> 0 and err == io.EOF at the end of file, and those m bytes will be ignored by
this loop. I think that you want something like

for {
  m, err := r.Read(b.buf[b.n:])
  b.n += m
  n += m
  // r.Read can return m > 0 && err != nil, such as at end of file.
  // We need to flush those m bytes before returning.
  if err1 := b.Flush(); err1 != nil {
    return n, err1
  }
  if err != nil {
    if err == io.EOF {
      err = nil
    }
    return n, err
  }
}
panic("unreachable")

A test of this (m > 0 && err != nil) code path would be good.

https://codereview.appspot.com/6565056/diff/5001/src/pkg/bufio/bufio_test.go
File src/pkg/bufio/bufio_test.go (right):

https://codereview.appspot.com/6565056/diff/5001/src/pkg/bufio/bufio_test.go#...
src/pkg/bufio/bufio_test.go:778: }
Add a blank line after this.

[chaten, 2012-09-28 06:12:08]

https://codereview.appspot.com/6565056/diff/5001/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

https://codereview.appspot.com/6565056/diff/5001/src/pkg/bufio/bufio.go#newco...
src/pkg/bufio/bufio.go:582: for m, err = b.readBuf(r); m > 0 && err == nil; m,
err = b.readBuf(r) {
On 2012/09/28 04:49:24, nigeltao wrote:
> This isn't right. "godoc io Reader" says that it's valid for r.Read to return
m
> > 0 and err == io.EOF at the end of file, and those m bytes will be ignored by
> this loop. I think that you want something like
> 
> for {
>   m, err := r.Read(b.buf[b.n:])
>   b.n += m
>   n += m
>   // r.Read can return m > 0 && err != nil, such as at end of file.
>   // We need to flush those m bytes before returning.
>   if err1 := b.Flush(); err1 != nil {
>     return n, err1
>   }
>   if err != nil {
>     if err == io.EOF {
>       err = nil
>     }
>     return n, err
>   }
> }
> panic("unreachable")
> 
> A test of this (m > 0 && err != nil) code path would be good.

Done.

https://codereview.appspot.com/6565056/diff/5001/src/pkg/bufio/bufio_test.go
File src/pkg/bufio/bufio_test.go (right):

https://codereview.appspot.com/6565056/diff/5001/src/pkg/bufio/bufio_test.go#...
src/pkg/bufio/bufio_test.go:778: }
On 2012/09/28 04:49:24, nigeltao wrote:
> Add a blank line after this.

Done.

[chaten, 2012-09-28 06:12:33]

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

Please take another look.

[chaten, 2012-09-28 06:15:32]

https://codereview.appspot.com/6565056/diff/9001/src/pkg/bufio/bufio_test.go
File src/pkg/bufio/bufio_test.go (right):

https://codereview.appspot.com/6565056/diff/9001/src/pkg/bufio/bufio_test.go#...
src/pkg/bufio/bufio_test.go:782: r :=
NewReader(&onlyReader{bytes.NewBuffer(input)})
I made this change to make sure we are testing the path of WriteTo that occurs
when the underlying reader is not a WriterTo.

[nigeltao, 2012-10-04 12:34:46]

http://codereview.appspot.com/6565056/diff/9001/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

http://codereview.appspot.com/6565056/diff/9001/src/pkg/bufio/bufio.go#newcod...
src/pkg/bufio/bufio.go:581: var n, m int64
You could delete some of these declarations if you name the return variables.

http://codereview.appspot.com/6565056/diff/9001/src/pkg/bufio/bufio.go#newcod...
src/pkg/bufio/bufio.go:583: for m, rerr = b.readBuf(r); m > 0; m, rerr =
b.readBuf(r) {
I'd prefer to use a 0-part for loop instead of a 3-part one, and then inline the
readBuf method:

func (b *Writer) ReadFrom(r io.Reader) (n int64, err error) {
  if err = b.Flush(); err != nil {
    return 0, err
  }
  if w, ok := b.wr.(io.ReaderFrom); ok {
    return w.ReadFrom(r)
  }
  var m int
  for {
    m, err = r.Read(b.buf[b.n:])
    if m == 0 {
      break
    }
    b.n += m
    n += int64(m)
    if err1 := b.Flush(); err1 != nil {
      return n, err1
    }
    if err != nil {
      break
    }
  }
  if err == io.EOF {
    err = nil
  }
  return n, err
}

http://codereview.appspot.com/6565056/diff/9001/src/pkg/bufio/bufio_test.go
File src/pkg/bufio/bufio_test.go (right):

http://codereview.appspot.com/6565056/diff/9001/src/pkg/bufio/bufio_test.go#n...
src/pkg/bufio/bufio_test.go:844: continue //Don't want to spit out a large
amount of errors below
I don't think that the comment is necessary.

http://codereview.appspot.com/6565056/diff/9001/src/pkg/bufio/bufio_test.go#n...
src/pkg/bufio/bufio_test.go:846: for i, val := range b.Bytes() {
I'd say
if got, want := b.String(), string(input); got != want {
  t.Errorf(ws[%d], rs[%d]:\ngot  %q\nwant %q\n", wi, ri, got, want)
}

[chaten, 2012-10-18 06:16:04]

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

Please take another look.

[nigeltao, 2012-10-18 06:25:11]

https://codereview.appspot.com/6565056/diff/13001/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

https://codereview.appspot.com/6565056/diff/13001/src/pkg/bufio/bufio.go#newc...
src/pkg/bufio/bufio.go:599: func (b *Writer) readBuf(r io.Reader) (int64, error)
{
Delete this unused method.

[chaten, 2012-10-18 08:02:24]

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

Please take another look.

[nigeltao, 2012-10-19 00:20:46]

LGTM.

[nigeltao, 2012-10-19 00:23:32]

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

bufio: Implement io.ReaderFrom for (*Writer).

This is part 2 of 2 for issue 4028.

benchmark                        old ns/op    new ns/op    delta
BenchmarkWriterCopyOptimal           53293        28326  -46.85%
BenchmarkWriterCopyUnoptimal         53757        30537  -43.19%
BenchmarkWriterCopyNoReadFrom        53192        36642  -31.11%

Fixes issue 4028.

R=nigeltao
CC=golang-dev
http://codereview.appspot.com/6565056

Committer: Nigel Tao <nigeltao@golang.org>

[chaten, 2012-11-20 22:00:40]

*** Abandoned ***