code review 6548047: bufio: Implement io.WriterTo for (*Reader) (issue 6548047) - golang-dev

The old Google Groups will be going away soon, but your browser is incompatible with the new version.
Message from discussion code review 6548047: bufio: Implement io.WriterTo for (*Reader) (issue 6548047)
Received-SPF: pass (google.com: domain of 3HjZhUA0JCpc3F45I5M95N-8I7D19C.3FD7FC1E7-45M7FF7C57IFLGJ....@m3kw2wvrgufz5godrsrytgd7.apphosting.bounces.google.com designates 209.85.212.72 as permitted sender) client-ip=209.85.212.72;
MIME-Version: 1.0
Reply-To: mcha...@gmail.com, d...@cheney.net, nigel...@golang.org, 
	r...@golang.org, bradf...@golang.org, golang-dev@googlegroups.com, 
	re...@codereview-hr.appspotmail.com
Message-ID: <047d7b6da0f80d1e8704ca7f549e@google.com>
Date: Tue, 25 Sep 2012 04:42:06 +0000
Subject: Re: code review 6548047: bufio: Implement io.WriterTo for (*Reader)
 (issue 6548047)
From: mcha...@gmail.com
To: d...@cheney.net, nigel...@golang.org, r...@golang.org, bradf...@golang.org
Cc: golang-dev@googlegroups.com, re...@codereview-hr.appspotmail.com
Content-Type: text/plain; charset=ISO-8859-1; format=flowed; delsp=yes


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

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio.go#newcode378
src/pkg/bufio/bufio.go:378: // WriteTo first writes the contents of the
buffer into the writer,
On 2012/09/24 15:34:09, rsc wrote:
> This describes how good the implementation is. The reader should
assume it is
> good. The coment should say what the function's effect is.

> // WriteTo copies data from the reader into the writer w until there's
no more
> // data to write or an error occurs. It returns the number of bytes
written
> // and the encountered error, if any.

> (mostly lifted from io.WriterTo's docs)


Didn't notice Tip io.WriterTo had docs, will go with brad's suggestion.

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio.go#newcode382
src/pkg/bufio/bufio.go:382: func (b *Reader) WriteTo(wtr io.Writer) (n
int64, err error) {
On 2012/09/24 15:34:09, rsc wrote:
> Writers are almost always called w, not wtr.

Noted

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio.go#newcode396
src/pkg/bufio/bufio.go:396: b.fill()
On 2012/09/24 15:34:09, rsc wrote:
> The sequencing here is a little tricky. You want to delay processing
the read
> error until after you write what did get read, as a write error then
would
> correspond to an earlier input byte than the read error. I think this
would
> work:

> for b.fill(); b.r < b.w; b.fill() {
>      m, err := b.writeTo(w)
>      n += m
>      if err != nil {
>          return n, err
>      }
> }
> if b.err == io.EOF {
>      b.err = nil
> }
> return n, b.readErr()

Fixed and added tests for the various permutations of errors.

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio.go#newcode416
src/pkg/bufio/bufio.go:416: func (b *Reader) writeBufTo(wtr io.Writer)
(int64, error) {
On 2012/09/24 15:34:09, rsc wrote:
> writeTo is fine. the 'Buf' is unnecessary.
> Also just call the writer w.

I'll rename it, but I don't think its the right decision. I feel like
the WriteTo code is less clear now that there is a private writeTo
method that only deals with the buffer. writeBufTo tells the reader that
it only is dealing with what is buffered.

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

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio_test.go#newcode767
src/pkg/bufio/bufio_test.go:767: input := make([]byte, 8096)
On 2012/09/24 15:34:09, rsc wrote:
> Interesting constant. Did you mean 8<<10 (8192)?

Ugh, thats embarrassing. Fixed.

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio_test.go#newcode771
src/pkg/bufio/bufio_test.go:771: inputRdr := bytes.NewBuffer(input)
On 2012/09/24 15:34:09, rsc wrote:
> You never use this variable again. Eliminate it:

> r := NewReader(bytes.NewReader(input))

Done

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio_test.go#newcode775
src/pkg/bufio/bufio_test.go:775:
On 2012/09/24 15:34:09, rsc wrote:
> Can collapse the next 8 lines into

> if n != int64(len(input)) || err != nil {
>      t.Fatalf("r.WriteTo(w) = %d, %v, want %d, nil", n, err,
len(input))
> }

Moved n,err := into the initializer block of the if as well

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio_test.go#newcode786
src/pkg/bufio/bufio_test.go:786: t.Errorf("Invalid byte written at index
%d. Expected %d, was %d.", i, input[i], val)
On 2012/09/24 15:34:09, rsc wrote:
> Echo the values back in the same order as the comparison, and use Go
syntax when
> possible, like in the new Fatalf in the last comment.
> t.Errorf("after write: out[%#x] = %#x, want %#x", i, val, input[i])

Done

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio_test.go#newcode791
src/pkg/bufio/bufio_test.go:791: //Indirect reader masks any other
interface a reader might implement, like io.WriterTo
On 2012/09/24 15:34:09, rsc wrote:
> Please call this 'onlyReader'. The type name should appear verbatim,
without
> additional spaces or case changes:

> // An onlyReader only implements io.Reader, no matter what other
methods
> // the underlying implementation may have.

Done and ditto for the writer.

http://codereview.appspot.com/6548047/
Create a group - Google Groups - Google Home - Terms of Service - Privacy Policy
Account Options
