code review 5639057: compress/gzip: NewWriter no longer returns an error.

compress: make flate, gzip and zlib's NewWriterXxx functions all return
(*Writer, error) if they take a compression level, and *Writer otherwise.
Rename gzip's Compressor and Decompressor to Writer and Reader, similar to
flate and zlib.

Clarify commentary when writing gzip metadata that is not representable
as Latin-1, and fix io.EOF comment bug.

Also refactor gzip_test to be more straightforward.

Fixes issue 2839.

[nigeltao, 2012-02-08 05:18:18]

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

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

[r, 2012-02-08 05:20:00]

missing go1 release note

[nigeltao, 2012-02-08 05:48:08]

On 8 February 2012 16:20,  <r@golang.org> wrote:
> missing go1 release note

Done.

[rsc1, 2012-02-08 05:51:20]

http://codereview.appspot.com/5639057/diff/1002/src/pkg/compress/gzip/gzip.go
File src/pkg/compress/gzip/gzip.go (right):

http://codereview.appspot.com/5639057/diff/1002/src/pkg/compress/gzip/gzip.go...
src/pkg/compress/gzip/gzip.go:54: // level is the compression level, which can
be DefaultCompression, NoCompression,
Sentences begin with capital letters.

// The compression level can be ...

But now you also have to define what happens if it is not.

[nigeltao, 2012-02-08 06:12:04]

On 8 February 2012 16:51,  <rsc@google.com> wrote:
> // The compression level can be ...
>
> But now you also have to define what happens if it is not.

OK, I will fix compress/flate in this CL to no longer drop the error
if a bad level is passed.

I propose that each of compress/{flate,gzip,zlib} have two functions:
NewWriter does not return an error, NewWriterXxx does:

flate.NewWriter(w io.Writer) *Writer
flate.NewWriterLevelDict(w io.Writer, level int, dict []byte) (*Writer, error)
gzip.NewWriter(w io.Writer) *Writer
gzip.NewWriterLevel(w io.Writer, level int) (*Writer, error)
zlib.NewWriter(w io.Writer) *Writer
zlib.NewWriterLevelDict(w io.Writer, level int, dict []byte) (*Writer, error)

The flate and zlib functions that take a level but not a dict will be removed.

gzip's Compressor and Decompressor types will be renamed to Writer and Reader.

WDYT?

[rsc1, 2012-02-08 06:21:50]

I'd rather not force Dict just to use Level.
Level is common; Dict is rare.

[nigeltao, 2012-02-08 21:40:22]

On 8 February 2012 17:21, Russ Cox <rsc@google.com> wrote:
> I'd rather not force Dict just to use Level.
> Level is common; Dict is rare.

OK, how about all three NewWriter funcs take a level, and return an error:

flate.NewWriter(w io.Writer, level int) (*Writer, error)
flate.NewWriterLevelDict(w io.Writer, level int, dict []byte) (*Writer, error)
gzip.NewWriter(w io.Writer, level int) (*Writer, error)
zlib.NewWriter(w io.Writer, level int) (*Writer, error)
zlib.NewWriterLevelDict(w io.Writer, level int, dict []byte) (*Writer, error)

[rsc1, 2012-02-08 21:45:04]

On Wed, Feb 8, 2012 at 16:40, Nigel Tao <nigeltao@golang.org> wrote:
> OK, how about all three NewWriter funcs take a level, and return an error:
>
> flate.NewWriter(w io.Writer, level int) (*Writer, error)
> flate.NewWriterLevelDict(w io.Writer, level int, dict []byte) (*Writer, error)
> gzip.NewWriter(w io.Writer, level int) (*Writer, error)
> zlib.NewWriter(w io.Writer, level int) (*Writer, error)

But the whole point of this cleanup was to remove
the error from common cases.  If NewWriter takes a level,
then you get these cannot-happen errors to check when
the level is DefaultCompression (which it will be often).

For flate that's not a big deal, but for gzip and zlib I think it is.
Flate is an implementation package.

 // document that if level is in range then err will be nil
 flate.NewWriter(w io.Writer, level int, dict []byte) (*Writer, error)

 gzip.NewWriter(w io.Writer) *Writer
 gzip.NewWriterLevel(w io.Writer, level int) (*Writer, error)

 zlib.NewWriter(w io.Writer) *Writer
 zlib.NewWriterLevel(w io.Writer, level int) (*Writer, error)

?
Russ

[nigeltao, 2012-02-08 22:36:23]

On 9 February 2012 08:45, Russ Cox <rsc@google.com> wrote:
>  // document that if level is in range then err will be nil
>  flate.NewWriter(w io.Writer, level int, dict []byte) (*Writer, error)
>
>  gzip.NewWriter(w io.Writer) *Writer
>  gzip.NewWriterLevel(w io.Writer, level int) (*Writer, error)
>
>  zlib.NewWriter(w io.Writer) *Writer
>  zlib.NewWriterLevel(w io.Writer, level int) (*Writer, error)

OK with me, plus (for completeness):
flate.NewWriterDict(w io.Writer, level int, dict []byte) (*Writer, error)
zlib.NewWriterLevelDict(w io.Writer, level int, dict []byte) (*Writer, error)

[rsc1, 2012-02-08 22:36:46]

lgtm

[nigeltao, 2012-02-09 02:48:52]

On 9 February 2012 08:45, Russ Cox <rsc@google.com> wrote:
>  flate.NewWriter(w io.Writer, level int, dict []byte) (*Writer, error)
>
>  gzip.NewWriter(w io.Writer) *Writer
>  gzip.NewWriterLevel(w io.Writer, level int) (*Writer, error)
>
>  zlib.NewWriter(w io.Writer) *Writer
>  zlib.NewWriterLevel(w io.Writer, level int) (*Writer, error)

It turns out that zlib.NewWriter has to return an error, since it
writes the ZLIB header to the underlying writer.

Given that, it looked odd for gzip.NewWriter to not return an error
when all the other NewWriterXxx functions did.

I've changed it so all such functions return (*Writer, error), but
gzip.NewWriter's doc comment says that the error is always nil. If you
would still prefer that gzip.NewWriter returned just *Writer, then
I'll change it back.

PTAL.

[bradfitz, 2012-02-09 02:57:15]

zlib.NewWriter could write its header on first Write or Close?

On Wed, Feb 8, 2012 at 6:48 PM, Nigel Tao <nigeltao@golang.org> wrote:

> On 9 February 2012 08:45, Russ Cox <rsc@google.com> wrote:
> >  flate.NewWriter(w io.Writer, level int, dict []byte) (*Writer, error)
> >
> >  gzip.NewWriter(w io.Writer) *Writer
> >  gzip.NewWriterLevel(w io.Writer, level int) (*Writer, error)
> >
> >  zlib.NewWriter(w io.Writer) *Writer
> >  zlib.NewWriterLevel(w io.Writer, level int) (*Writer, error)
>
> It turns out that zlib.NewWriter has to return an error, since it
> writes the ZLIB header to the underlying writer.
>
> Given that, it looked odd for gzip.NewWriter to not return an error
> when all the other NewWriterXxx functions did.
>
> I've changed it so all such functions return (*Writer, error), but
> gzip.NewWriter's doc comment says that the error is always nil. If you
> would still prefer that gzip.NewWriter returned just *Writer, then
> I'll change it back.
>
> PTAL.
>

[rsc1, 2012-02-09 03:27:33]

On Wed, Feb 8, 2012 at 21:48, Nigel Tao <nigeltao@golang.org> wrote:
> It turns out that zlib.NewWriter has to return an error, since it
> writes the ZLIB header to the underlying writer.
>
> Given that, it looked odd for gzip.NewWriter to not return an error
> when all the other NewWriterXxx functions did.
>
> I've changed it so all such functions return (*Writer, error), but
> gzip.NewWriter's doc comment says that the error is always nil. If you
> would still prefer that gzip.NewWriter returned just *Writer, then
> I'll change it back.

Please change it back, and zlib too.
zlib can postpone the writing of the header
(or even just the reporting of the error, if you
want to minimize the code delta) until the
Write.  Logically, that seems fine: the Write
reports a Write error.

Russ

[nigeltao, 2012-02-09 03:36:35]

On 9 February 2012 14:27, Russ Cox <rsc@google.com> wrote:
> Please change it back, and zlib too.
> zlib can postpone the writing of the header
> (or even just the reporting of the error, if you
> want to minimize the code delta) until the
> Write.  Logically, that seems fine: the Write
> reports a Write error.

Duh, of course. Done.

[rsc, 2012-02-10 00:33:48]

LGTM modulo comments.
Maybe someone in Sydney can reread the comments
before you submit.  Please file an issue to create a gofix
for this (the renamings, and maybe also w, _ := NewWriter -> w := NewWriter,
and flagging the others).

http://codereview.appspot.com/5639057/diff/3011/src/pkg/compress/flate/deflat...
File src/pkg/compress/flate/deflate_test.go (right):

http://codereview.appspot.com/5639057/diff/3011/src/pkg/compress/flate/deflat...
src/pkg/compress/flate/deflate_test.go:84: w, _ := NewWriter(&buf, h.level)
Check error please.

http://codereview.appspot.com/5639057/diff/3011/src/pkg/compress/gzip/gzip.go
File src/pkg/compress/gzip/gzip.go (right):

http://codereview.appspot.com/5639057/diff/3011/src/pkg/compress/gzip/gzip.go...
src/pkg/compress/gzip/gzip.go:47: // It is the caller's responsibility to call
Close on the WriteCloser when done.
Please move this text and the next paragraph up to the NewWriter comment.
People will be looking at that one more often.

// NewWriterLevel is like NewWriter but specifies the compression level
// instead of assuming DefaultCompression.
//
// The compression level can ...

http://codereview.appspot.com/5639057/diff/3011/src/pkg/compress/gzip/gzip.go...
src/pkg/compress/gzip/gzip.go:104: needconv := false
// gzip stores Latin-1 strings; error if non-Latin-1; convert if non-ASCII

http://codereview.appspot.com/5639057/diff/3011/src/pkg/compress/zlib/writer.go
File src/pkg/compress/zlib/writer.go (right):

http://codereview.appspot.com/5639057/diff/3011/src/pkg/compress/zlib/writer....
src/pkg/compress/zlib/writer.go:49: // NewWriterLevelDict creates a new Writer
that satisfies writes by compressing
Same comment about moving the general text up to NewWriter.

http://codereview.appspot.com/5639057/diff/3011/src/pkg/crypto/rand/rand_test.go
File src/pkg/crypto/rand/rand_test.go (right):

http://codereview.appspot.com/5639057/diff/3011/src/pkg/crypto/rand/rand_test...
src/pkg/crypto/rand/rand_test.go:25: f, _ := flate.NewWriter(&z, 5)
NewWriter(&z) should be fine

[r, 2012-02-10 01:53:57]

LGTM after fixing .html

http://codereview.appspot.com/5639057/diff/3011/doc/go1.tmpl
File doc/go1.tmpl (right):

http://codereview.appspot.com/5639057/diff/3011/doc/go1.tmpl#newcode795
doc/go1.tmpl:795: <em>Updating</em>:
.html and .tmpl differ - run make

http://codereview.appspot.com/5639057/diff/3011/src/pkg/compress/gzip/gzip.go
File src/pkg/compress/gzip/gzip.go (right):

http://codereview.appspot.com/5639057/diff/3011/src/pkg/compress/gzip/gzip.go...
src/pkg/compress/gzip/gzip.go:52: // UTF-8 strings in Go, but the underlying
format is NUL-terminated ISO 8859-1
UTF-8 isn't relevant, quite. strings aren't UTF-8 necessarily.  maybe
"general Go strings"

s/is/requires/

http://codereview.appspot.com/5639057/diff/3011/src/pkg/compress/gzip/gzip.go...
src/pkg/compress/gzip/gzip.go:57: // integer value between BestSpeed and
BestCompression (inclusive). The error
s/[()]//g

[nigeltao, 2012-02-10 07:33:22]

Submitting...

http://codereview.appspot.com/5639057/diff/3011/src/pkg/compress/flate/deflat...
File src/pkg/compress/flate/deflate_test.go (right):

http://codereview.appspot.com/5639057/diff/3011/src/pkg/compress/flate/deflat...
src/pkg/compress/flate/deflate_test.go:84: w, _ := NewWriter(&buf, h.level)
On 2012/02/10 00:33:48, rsc wrote:
> Check error please.

Done.

http://codereview.appspot.com/5639057/diff/3011/src/pkg/compress/gzip/gzip.go
File src/pkg/compress/gzip/gzip.go (right):

http://codereview.appspot.com/5639057/diff/3011/src/pkg/compress/gzip/gzip.go...
src/pkg/compress/gzip/gzip.go:47: // It is the caller's responsibility to call
Close on the WriteCloser when done.
On 2012/02/10 00:33:48, rsc wrote:
> Please move this text and the next paragraph up to the NewWriter comment.
> People will be looking at that one more often.
> 
> // NewWriterLevel is like NewWriter but specifies the compression level
> // instead of assuming DefaultCompression.
> //
> // The compression level can ...

Done.

http://codereview.appspot.com/5639057/diff/3011/src/pkg/compress/gzip/gzip.go...
src/pkg/compress/gzip/gzip.go:52: // UTF-8 strings in Go, but the underlying
format is NUL-terminated ISO 8859-1
On 2012/02/10 01:53:57, r wrote:
> UTF-8 isn't relevant, quite. strings aren't UTF-8 necessarily.  maybe
> "general Go strings"

The wording is tricky. The in-memory Go string is in UTF-8 format. The on-disk
GZIP bytes are in Latin-1 format. I want to explicitly mention UTF-8 to avoid
people trying to use a Latin-1 Go string literal like "Gau\xdf".


> s/is/requires/

Done.

http://codereview.appspot.com/5639057/diff/3011/src/pkg/compress/gzip/gzip.go...
src/pkg/compress/gzip/gzip.go:57: // integer value between BestSpeed and
BestCompression (inclusive). The error
On 2012/02/10 01:53:57, r wrote:
> s/[()]//g

Done.

http://codereview.appspot.com/5639057/diff/3011/src/pkg/compress/gzip/gzip.go...
src/pkg/compress/gzip/gzip.go:104: needconv := false
On 2012/02/10 00:33:48, rsc wrote:
> // gzip stores Latin-1 strings; error if non-Latin-1; convert if non-ASCII

Done.

http://codereview.appspot.com/5639057/diff/3011/src/pkg/compress/zlib/writer.go
File src/pkg/compress/zlib/writer.go (right):

http://codereview.appspot.com/5639057/diff/3011/src/pkg/compress/zlib/writer....
src/pkg/compress/zlib/writer.go:49: // NewWriterLevelDict creates a new Writer
that satisfies writes by compressing
On 2012/02/10 00:33:48, rsc wrote:
> Same comment about moving the general text up to NewWriter.

Done.

http://codereview.appspot.com/5639057/diff/3011/src/pkg/crypto/rand/rand_test.go
File src/pkg/crypto/rand/rand_test.go (right):

http://codereview.appspot.com/5639057/diff/3011/src/pkg/crypto/rand/rand_test...
src/pkg/crypto/rand/rand_test.go:25: f, _ := flate.NewWriter(&z, 5)
On 2012/02/10 00:33:48, rsc wrote:
> NewWriter(&z) should be fine

No, flate.NewWriter requires a compression level.

[nigeltao, 2012-02-10 07:49:29]

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

compress: make flate, gzip and zlib's NewWriterXxx functions all return
(*Writer, error) if they take a compression level, and *Writer otherwise.
Rename gzip's Compressor and Decompressor to Writer and Reader, similar to
flate and zlib.

Clarify commentary when writing gzip metadata that is not representable
as Latin-1, and fix io.EOF comment bug.

Also refactor gzip_test to be more straightforward.

Fixes issue 2839.

R=rsc, r, rsc, bradfitz
CC=golang-dev
http://codereview.appspot.com/5639057

[r, 2012-02-10 12:05:50]

you didn't address my comments.

-rob

[nigeltao, 2012-02-10 22:23:16]

On 10 February 2012 23:05, Rob 'Commander' Pike <r@golang.org> wrote:
> you didn't address my comments.

I thought I did. Which ones did I miss?

[r2, 2012-02-10 22:33:03]

On 11/02/2012, at 9:23 AM, Nigel Tao wrote:

> On 10 February 2012 23:05, Rob 'Commander' Pike <r@golang.org> wrote:
>> you didn't address my comments.
> 
> I thought I did. Which ones did I miss?

hmmm..... maybe it was in some other context. i'll send again.

-rob

[r, 2012-02-10 22:33:20]

http://codereview.appspot.com/5639057/diff/13001/doc/go1.tmpl
File doc/go1.tmpl (right):

http://codereview.appspot.com/5639057/diff/13001/doc/go1.tmpl#newcode788
doc/go1.tmpl:788: In Go 1, the NewWriterXxx functions in compress/flate,
compress/gzip and
<code>NewWriterXxx</code>
also link the packages.

http://codereview.appspot.com/5639057/diff/13001/doc/go1.tmpl#newcode789
doc/go1.tmpl:789: compress/zlib all return (*Writer, error) if they take a
compression level,
<code> throughout here.