code review 6760045: io: add ByteWriter interface

io: add ByteWriter interface

API change.

[bradfitz, 2012-10-25 00:45:31]

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

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

[bradfitz, 2012-10-25 00:47:02]

API proposal.  Mostly for symmetry with io.WriteString, but also because
I've now seen this a number of places.

On Wed, Oct 24, 2012 at 5:45 PM, <bradfitz@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> io: add a WriteByte function, like WriteString
>
> Please review this at
http://codereview.appspot.com/**6760045/<http://codereview.appspot.com/6760045/>
>
> Affected files:
>   M src/pkg/io/io.go
>
>
> Index: src/pkg/io/io.go
> ==============================**==============================**=======
> --- a/src/pkg/io/io.go
> +++ b/src/pkg/io/io.go
> @@ -251,6 +251,20 @@
>         return w.Write([]byte(s))
>  }
>
> +type byteWriter interface {
> +       WriteByte(c byte) error
> +}
> +
> +// WriteByte writes the byte c to w.
> +// If w already implements a WriteByte method, it is invoked directly.
> +func WriteByte(w Writer, c byte) error {
> +       if bw, ok := w.(byteWriter); ok {
> +               return bw.WriteByte(c)
> +       }
> +       _, err := w.Write([]byte{c})
> +       return err
> +}
> +
>  // ReadAtLeast reads from r into buf until it has read at least min bytes.
>  // It returns the number of bytes copied and an error if fewer bytes were
> read.
>  // The error is EOF only if no bytes were read.
>
>
>

[dsymonds, 2012-10-25 00:48:55]

LGTM

https://codereview.appspot.com/6760045/diff/3002/src/pkg/io/io.go
File src/pkg/io/io.go (right):

https://codereview.appspot.com/6760045/diff/3002/src/pkg/io/io.go#newcode259
src/pkg/io/io.go:259: // If w already implements a WriteByte method, it is
invoked directly.
s/already //

(and for WriteString too)

[nigeltao, 2012-10-25 04:31:59]

I am afraid that this will encourage inefficient code that repeatedly calls
io.WriteByte.

Instead, programs should instead sniff an io.Writer for interface{
WriteByte(byte) error } and use a bufio.Writer if not supported.

For example, this is what compress/lzw/writer.go, exp/html/render.go and
image/jpeg/writer.go all do.

FWIW, I was about to mail a change that added the io.ByteWriter interface type
but nothing else.

https://codereview.appspot.com/6760045/diff/3002/src/pkg/io/io.go
File src/pkg/io/io.go (right):

https://codereview.appspot.com/6760045/diff/3002/src/pkg/io/io.go#newcode254
src/pkg/io/io.go:254: type byteWriter interface {
Export this type? We already have io.ByteReader.

[bradfitz, 2012-10-25 15:43:00]

On Wed, Oct 24, 2012 at 9:31 PM, <nigeltao@golang.org> wrote:

> I am afraid that this will encourage inefficient code that repeatedly
> calls io.WriteByte.
>

Yeah, I was worrying about that too a bit.  I was hoping it could be
optimized to be an acceptable pattern, though, but maybe it can't.


> Instead, programs should instead sniff an io.Writer for interface{
> WriteByte(byte) error } and use a bufio.Writer if not supported.
>

Fair enough.


>
> For example, this is what compress/lzw/writer.go, exp/html/render.go and
> image/jpeg/writer.go all do.
>
> FWIW, I was about to mail a change that added the io.ByteWriter
> interface type but nothing else.


K, let's just do that.


>
>
https://codereview.appspot.**com/6760045/diff/3002/src/pkg/**io/io.go<https:/...
> File src/pkg/io/io.go (right):
>
> https://codereview.appspot.**com/6760045/diff/3002/src/pkg/**
>
io/io.go#newcode254<https://codereview.appspot.com/6760045/diff/3002/src/pkg/io/io.go#newcode254>
> src/pkg/io/io.go:254: type byteWriter interface {
> Export this type? We already have io.ByteReader.
>
>
https://codereview.appspot.**com/6760045/<https://codereview.appspot.com/6760...
>

[rsc, 2012-10-25 21:37:03]

No. This is the wrong thing to do.

You don't want to do this on every byte. You want to do this once, and
if the underlying writer cannot take bytes, you wrap it in something
that can, like by calling bufio.NewWriter.

Russ

[rsc, 2012-10-25 21:37:13]

NOT LGTM

[bradfitz, 2012-10-29 10:44:01]

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

Please take another look.

[r, 2012-10-29 21:14:15]

http://codereview.appspot.com/6760045/diff/13001/src/pkg/io/io.go
File src/pkg/io/io.go (right):

http://codereview.appspot.com/6760045/diff/13001/src/pkg/io/io.go#newcode223
src/pkg/io/io.go:223: // are buffered in-memory.
two weasel-word adverbs in two sentences.
the interface cannot guarantee that it is efficient, so the documentation for
the interface should just say what it does.
by the same token, the interface cannot guarantee how the implementations work.

what are you trying to achieve in this comment? what's wrong with plain,

ByteWriter is the interface that wraps the WriteByte method.

-rob

[bradfitz, 2012-10-29 21:19:05]

That's fine.

I was trying to discourage dumb implementations. But whatever.
 On Oct 29, 2012 10:14 PM, <r@golang.org> wrote:

>
>
http://codereview.appspot.com/**6760045/diff/13001/src/pkg/io/**io.go<http://...
> File src/pkg/io/io.go (right):
>
> http://codereview.appspot.com/**6760045/diff/13001/src/pkg/io/**
>
io.go#newcode223<http://codereview.appspot.com/6760045/diff/13001/src/pkg/io/io.go#newcode223>
> src/pkg/io/io.go:223: // are buffered in-memory.
> two weasel-word adverbs in two sentences.
> the interface cannot guarantee that it is efficient, so the
> documentation for the interface should just say what it does.
> by the same token, the interface cannot guarantee how the
> implementations work.
>
> what are you trying to achieve in this comment? what's wrong with plain,
>
> ByteWriter is the interface that wraps the WriteByte method.
>
> -rob
>
>
http://codereview.appspot.com/**6760045/<http://codereview.appspot.com/6760045/>
>

[bradfitz, 2012-10-29 21:55:08]

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

Please take another look.

[nigeltao, 2012-10-29 22:47:36]

LGTM.

[bradfitz, 2012-10-30 09:51:36]

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

io: add ByteWriter interface

API change.

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