code review 6632046: bytes, strings: add (*Reader).WriteTo

bytes, strings: add (*Reader).WriteTo

Fixes issue 4031.

[eds, 2012-10-08 21:58:12]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2012-10-09 04:46:53]

https://codereview.appspot.com/6632046/diff/1002/src/pkg/bytes/reader.go
File src/pkg/bytes/reader.go (right):

https://codereview.appspot.com/6632046/diff/1002/src/pkg/bytes/reader.go#newc...
src/pkg/bytes/reader.go:131: m, e := w.Write(b)
I can see the advantage of this CL, but I'm a little nervous about this part. 
It permits dumb or malicious Writers to manipulate my Reader's read-only state.

But I guess that's really a problem with the io.Writer interface taking a
mutable argument.

https://codereview.appspot.com/6632046/diff/1002/src/pkg/bytes/reader.go#newc...
src/pkg/bytes/reader.go:137: if e != nil {
just use err instead of e

https://codereview.appspot.com/6632046/diff/1002/src/pkg/strings/reader.go
File src/pkg/strings/reader.go (right):

https://codereview.appspot.com/6632046/diff/1002/src/pkg/strings/reader.go#ne...
src/pkg/strings/reader.go:13: // A Reader implements the io.Reader, io.ReaderAt,
io.Seeker,
mention WriterTo?

[eds, 2012-10-09 05:58:09]

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

Please take another look.

[bradfitz, 2012-10-09 18:23:56]

https://codereview.appspot.com/6632046/diff/7002/src/pkg/bytes/reader.go
File src/pkg/bytes/reader.go (right):

https://codereview.appspot.com/6632046/diff/7002/src/pkg/bytes/reader.go#newc...
src/pkg/bytes/reader.go:133: panic("bytes.Reader.WriteTo: invalid Write count")
I'm not sure this is panic-worthy.  Why not just return an error here?

https://codereview.appspot.com/6632046/diff/7002/src/pkg/bytes/reader.go#newc...
src/pkg/bytes/reader.go:137: if err != nil {
delete these three lines, and

https://codereview.appspot.com/6632046/diff/7002/src/pkg/bytes/reader.go#newc...
src/pkg/bytes/reader.go:140: if m != len(b) {
if err == nil && m != len(b) {
    err = io.ErrShortWrite
}
return

https://codereview.appspot.com/6632046/diff/7002/src/pkg/bytes/reader.go#newc...
src/pkg/bytes/reader.go:143: return n, nil
just return

https://codereview.appspot.com/6632046/diff/7002/src/pkg/strings/reader.go
File src/pkg/strings/reader.go (right):

https://codereview.appspot.com/6632046/diff/7002/src/pkg/strings/reader.go#ne...
src/pkg/strings/reader.go:132: panic("strings.Reader.WriteTo: invalid
WriteString count")
same comments from other file

[eds, 2012-10-09 18:27:31]

https://codereview.appspot.com/6632046/diff/7002/src/pkg/bytes/reader.go
File src/pkg/bytes/reader.go (right):

https://codereview.appspot.com/6632046/diff/7002/src/pkg/bytes/reader.go#newc...
src/pkg/bytes/reader.go:133: panic("bytes.Reader.WriteTo: invalid Write count")
On 2012/10/09 18:23:57, bradfitz wrote:
> I'm not sure this is panic-worthy.  Why not just return an error here?

I agree, but I was following what happens in bytes.Buffer. Want to change that
one too?

[bradfitz, 2012-10-09 18:31:34]

On Tue, Oct 9, 2012 at 11:27 AM, <chickencha@gmail.com> wrote:

>
> https://codereview.appspot.**com/6632046/diff/7002/src/pkg/**
>
bytes/reader.go<https://codereview.appspot.com/6632046/diff/7002/src/pkg/bytes/reader.go>
> File src/pkg/bytes/reader.go (right):
>
> https://codereview.appspot.**com/6632046/diff/7002/src/pkg/**
>
bytes/reader.go#newcode133<https://codereview.appspot.com/6632046/diff/7002/src/pkg/bytes/reader.go#newcode133>
> src/pkg/bytes/reader.go:133: panic("bytes.Reader.WriteTo: invalid Write
> count")
> On 2012/10/09 18:23:57, bradfitz wrote:
>
>> I'm not sure this is panic-worthy.  Why not just return an error here?
>>
>
> I agree, but I was following what happens in bytes.Buffer. Want to
> change that one too?
>

Let's wait for others' opinions.

[remyoudompheng, 2012-10-09 21:58:45]

http://codereview.appspot.com/6632046/diff/7002/src/pkg/strings/reader.go
File src/pkg/strings/reader.go (right):

http://codereview.appspot.com/6632046/diff/7002/src/pkg/strings/reader.go#new...
src/pkg/strings/reader.go:130: m, e := io.WriteString(w, s)
if the writer has no WriteString() method it will allocate a large bytestring
and copy the whole string contents to it, whereas io.Copy will allocate a fixed
size buffer and reuse it, hence creating less garbage if the underlying string
is larg, but probably more garbage in most cases (32kB).

What do you think?

[bradfitz, 2012-10-09 22:04:47]

https://codereview.appspot.com/6632046/diff/7002/src/pkg/strings/reader.go
File src/pkg/strings/reader.go (right):

https://codereview.appspot.com/6632046/diff/7002/src/pkg/strings/reader.go#ne...
src/pkg/strings/reader.go:130: m, e := io.WriteString(w, s)
On 2012/10/09 21:58:46, remyoudompheng wrote:
> if the writer has no WriteString() method it will allocate a large bytestring
> and copy the whole string contents to it, whereas io.Copy will allocate a
fixed
> size buffer and reuse it, hence creating less garbage if the underlying string
> is larg, but probably more garbage in most cases (32kB).
> 
> What do you think?

If we were to do such an optimization, why here instead of just fixing
io.WriteString to do that if/when it makes sense?  Since WriteString and Copy
are in the same package, that 32kB constant can remain package-private.

But--- we don't know whether it's cheaper to to make a copy of a 5MB
string->[]byte or do a Write call.  What if the destination io.Writer is doing
slow network round-trips and each Write is 350 ms.  In that case it might be
better to just give it one large []byte and let it handle it.

[r, 2012-10-10 02:39:21]

http://codereview.appspot.com/6632046/diff/7002/src/pkg/strings/reader.go
File src/pkg/strings/reader.go (right):

http://codereview.appspot.com/6632046/diff/7002/src/pkg/strings/reader.go#new...
src/pkg/strings/reader.go:130: m, e := io.WriteString(w, s)
I lean towards leaving io.WriteString alone at least for now.  Strings can be
big but usually aren't, and it's nice to generate one syscall. Plus if you
decide to break it up you need heuristics about what size is correct. Plus most
of the guys where it really matters implement WriteString anyway.

But I'm not adamant and could be swayed.

But I agree that calling out to WriteString feels wrong here. However I think
Copy won't do it because it'll come right back to Reader.WriteTo, won't it?

[r, 2012-10-10 02:40:07]

I guess that wasn't very helpful. Apologies for that.

-rob

[bradfitz, 2012-10-10 02:58:28]

I think WriteString is fine. Safer, less surprising default. And not
changing behavior.

Thoughts though on panic versus error?
On Oct 9, 2012 7:40 PM, "Rob Pike" <r@golang.org> wrote:

> I guess that wasn't very helpful. Apologies for that.
>
> -rob
>

[eds, 2012-10-10 03:00:18]

http://codereview.appspot.com/6632046/diff/7002/src/pkg/strings/reader.go
File src/pkg/strings/reader.go (right):

http://codereview.appspot.com/6632046/diff/7002/src/pkg/strings/reader.go#new...
src/pkg/strings/reader.go:130: m, e := io.WriteString(w, s)
On 2012/10/10 02:39:21, r wrote:
> But I agree that calling out to WriteString feels wrong here. However I think
> Copy won't do it because it'll come right back to Reader.WriteTo, won't it?

One way or another I think it's important to call the Writer's WriteString
method when available to avoid allocating.

It's possible to call back into io.Copy without recursing if we wrap the Reader
in a struct that only has a Read method. net does something similar with its
unexported writerOnly type.

[r, 2012-10-10 03:16:22]

http://codereview.appspot.com/6632046/diff/7002/src/pkg/bytes/reader.go
File src/pkg/bytes/reader.go (right):

http://codereview.appspot.com/6632046/diff/7002/src/pkg/bytes/reader.go#newco...
src/pkg/bytes/reader.go:133: panic("bytes.Reader.WriteTo: invalid Write count")
it's the caller's error not an error from what this calls. i'm OK with panic

[eds, 2012-10-10 22:42:55]

Hello golang-dev@googlegroups.com, bradfitz@golang.org,
remyoudompheng@gmail.com, r@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2012-10-10 22:48:28]

LGTM after nits

Will give Rob a day or so for final comments before submitting (or anybody else
can)

https://codereview.appspot.com/6632046/diff/4007/src/pkg/bytes/reader_test.go
File src/pkg/bytes/reader_test.go (right):

https://codereview.appspot.com/6632046/diff/4007/src/pkg/bytes/reader_test.go...
src/pkg/bytes/reader_test.go:103: t.Errorf("got string %v; want %v", b.String(),
s)
%q for strings.  then you can see empty and gibberish.

https://codereview.appspot.com/6632046/diff/4007/src/pkg/strings/reader_test.go
File src/pkg/strings/reader_test.go (right):

https://codereview.appspot.com/6632046/diff/4007/src/pkg/strings/reader_test....
src/pkg/strings/reader_test.go:105: t.Errorf("got string %v; want %v",
b.String(), s)
%q

[eds, 2012-10-10 22:50:44]

Hello golang-dev@googlegroups.com, bradfitz@golang.org,
remyoudompheng@gmail.com, r@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[dfc, 2012-10-10 23:51:52]

LGTM with some minor grumbling about unnecessary named return values.

https://codereview.appspot.com/6632046/diff/9007/src/pkg/bytes/reader.go
File src/pkg/bytes/reader.go (right):

https://codereview.appspot.com/6632046/diff/9007/src/pkg/bytes/reader.go#newc...
src/pkg/bytes/reader.go:125: func (r *Reader) WriteTo(w io.Writer) (n int64, err
error) {
I'm not a fan of the named return values here. I think the method would be
clearer like this

...
n = int64(m)
if m != len(b) { 
   return n, io.ErrShortWrite
}
return n, nil

https://codereview.appspot.com/6632046/diff/9007/src/pkg/strings/reader.go
File src/pkg/strings/reader.go (right):

https://codereview.appspot.com/6632046/diff/9007/src/pkg/strings/reader.go#ne...
src/pkg/strings/reader.go:134: r.i += m
see previous comments in bytes.WriteTo()

[bradfitz, 2012-10-11 00:06:29]

https://codereview.appspot.com/6632046/diff/9007/src/pkg/bytes/reader.go
File src/pkg/bytes/reader.go (right):

https://codereview.appspot.com/6632046/diff/9007/src/pkg/bytes/reader.go#newc...
src/pkg/bytes/reader.go:125: func (r *Reader) WriteTo(w io.Writer) (n int64, err
error) {
On 2012/10/10 23:51:52, dfc wrote:
> I'm not a fan of the named return values here.

It's fine. It's better than fine, actually.  It's good because it's consistent
with everything else in package io, like:

http://golang.org/pkg/io/#Writer

[dfc, 2012-10-11 00:14:09]

Fair enough. Please ignore my comments.
On 11 Oct 2012 11:06, <bradfitz@golang.org> wrote:

>
> https://codereview.appspot.**com/6632046/diff/9007/src/pkg/**
>
bytes/reader.go<https://codereview.appspot.com/6632046/diff/9007/src/pkg/bytes/reader.go>
> File src/pkg/bytes/reader.go (right):
>
> https://codereview.appspot.**com/6632046/diff/9007/src/pkg/**
>
bytes/reader.go#newcode125<https://codereview.appspot.com/6632046/diff/9007/src/pkg/bytes/reader.go#newcode125>
> src/pkg/bytes/reader.go:125: func (r *Reader) WriteTo(w io.Writer) (n
> int64, err error) {
> On 2012/10/10 23:51:52, dfc wrote:
>
>> I'm not a fan of the named return values here.
>>
>
> It's fine. It's better than fine, actually.  It's good because it's
> consistent with everything else in package io, like:
>
> http://golang.org/pkg/io/#**Writer <http://golang.org/pkg/io/#Writer>
>
>
https://codereview.appspot.**com/6632046/<https://codereview.appspot.com/6632...
>

[r, 2012-10-12 01:36:38]

This CL breaks net/http.


--- FAIL: TestHeadResponses (0.00 seconds)
serve_test.go:548: 	on Copy, expected ErrBodyNotAllowed, got short write

[eds, 2012-10-12 01:45:13]

Hello golang-dev@googlegroups.com, bradfitz@golang.org,
remyoudompheng@gmail.com, r@golang.org, dave@cheney.net (cc:
golang-dev@googlegroups.com),

Please take another look.

[r, 2012-10-12 03:40:55]

LGTM
will submit

[r, 2012-10-12 03:43:57]

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

bytes, strings: add (*Reader).WriteTo

Fixes issue 4031.

R=golang-dev, bradfitz, remyoudompheng, r, dave
CC=golang-dev
http://codereview.appspot.com/6632046

Committer: Rob Pike <r@golang.org>