code review 4372047: archive/zip: add new type ReadCloser, make OpenReader r...

archive/zip: add new type ReadCloser, make OpenReader return it.
Fixes issue 1678.

[dchest, 2011-04-08 14:17:51]

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

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

[dchest, 2011-04-08 14:20:02]

Some alternative approaches:

* Make OpenReader return three values: Reader, os.File, os.Error.
* Add reference to zip file right into Reader type, add Close() method to it,
and ignore it when zip file is nil.
* Completely remove OpenReader method.

[rsc, 2011-04-08 14:22:49]

With the changes below, LGTM.
Leaving for adg.

http://codereview.appspot.com/4372047/diff/5001/src/pkg/archive/zip/reader.go
File src/pkg/archive/zip/reader.go (right):

http://codereview.appspot.com/4372047/diff/5001/src/pkg/archive/zip/reader.go...
src/pkg/archive/zip/reader.go:65: r, err := NewReader(f, fi.Size)
Instead if copying r later, it would be better to rename
most of NewReader to 

func (*Reader) init(io.ReaderAt, size int64) 

and then this function ends with

r := new(ReadCloser)
if err := r.init(f, fi.Size); err != nil {
    f.Close()
    return nil, err
}
return r, nil

and the new NewReader is

zr := new(Reader)
if err := zr.init(r, size); err != nil {
    return nil, err
}
return zr, nil

http://codereview.appspot.com/4372047/diff/5001/src/pkg/archive/zip/reader.go...
src/pkg/archive/zip/reader.go:98: // Close closes the Zip file, rendering it
unusable for I/O. It returns an error if any.
Delete "It returns an error if any."
The function signature says that.

[dchest, 2011-04-08 14:43:23]

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

Please take another look.

[dchest, 2011-04-08 14:44:27]

http://codereview.appspot.com/4372047/diff/5001/src/pkg/archive/zip/reader.go
File src/pkg/archive/zip/reader.go (right):

http://codereview.appspot.com/4372047/diff/5001/src/pkg/archive/zip/reader.go...
src/pkg/archive/zip/reader.go:65: r, err := NewReader(f, fi.Size)
On 2011/04/08 14:22:49, rsc wrote:
> Instead if copying r later, it would be better to rename
> most of NewReader to 
> 
> func (*Reader) init(io.ReaderAt, size int64) 
> 
> and then this function ends with
> 
> r := new(ReadCloser)
> if err := r.init(f, fi.Size); err != nil {
>     f.Close()
>     return nil, err
> }
> return r, nil
> 
> and the new NewReader is
> 
> zr := new(Reader)
> if err := zr.init(r, size); err != nil {
>     return nil, err
> }
> return zr, nil

Done.

http://codereview.appspot.com/4372047/diff/5001/src/pkg/archive/zip/reader.go...
src/pkg/archive/zip/reader.go:98: // Close closes the Zip file, rendering it
unusable for I/O. It returns an error if any.
On 2011/04/08 14:22:49, rsc wrote:
> Delete "It returns an error if any."
> The function signature says that.

Done.

[adg, 2011-04-10 01:23:27]

LGTM

Thanks.

[adg, 2011-04-10 01:23:51]

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

archive/zip: add new type ReadCloser, make OpenReader return it.
Fixes issue 1678.

R=adg, rsc
CC=golang-dev
http://codereview.appspot.com/4372047

Committer: Andrew Gerrand <adg@golang.org>