code review 5694085: archive/zip: stop using encoding/binary

archive/zip: stop using encoding/binary

[adg, 2012-02-27 05:23:34]

Hello golang-dev@googlegroups.com,

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

[r, 2012-02-27 05:24:54]

LGTM

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

http://codereview.appspot.com/5694085/diff/1/src/pkg/archive/zip/reader.go#ne...
src/pkg/archive/zip/reader.go:307: 
perhaps say why you're not using it (avoids reflection and we know the type and
it's easy)

[bradfitz, 2012-02-27 05:28:11]

Update the CL description to say how fast (in second granularity from your
wristwatch) faster the TestOver65kFiles test is now.


On Mon, Feb 27, 2012 at 4:23 PM, <adg@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg
>
>
> Description:
> archive/zip: stop using encoding/binary
>
> Please review this at
http://codereview.appspot.com/**5694085/<http://codereview.appspot.com/5694085/>
>
> Affected files:
>  M src/pkg/archive/zip/reader.go
>  M src/pkg/archive/zip/struct.go
>  M src/pkg/archive/zip/writer.go
>
>
>

[adg, 2012-02-27 05:29:27]

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

archive/zip: stop using encoding/binary

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

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

http://codereview.appspot.com/5694085/diff/1/src/pkg/archive/zip/reader.go#ne...
src/pkg/archive/zip/reader.go:307: 
On 2012/02/27 05:24:54, r wrote:
> perhaps say why you're not using it (avoids reflection and we know the type
and
> it's easy)

Done.

[bradfitz, 2012-02-27 05:33:15]

http://codereview.appspot.com/5694085/diff/1/src/pkg/archive/zip/writer.go
File src/pkg/archive/zip/writer.go (right):

http://codereview.appspot.com/5694085/diff/1/src/pkg/archive/zip/writer.go#ne...
src/pkg/archive/zip/writer.go:55: putUint32(b[:],
uint32(directoryHeaderSignature))
this would read nicer if each of these putFoo functions returned a new slice
that was the next position to write.

then it'd just be:

var buf [directoryHeaderLen]byte
b := buf[:]
b = putUint32(b, uint32(directoryHeaderSignature))
b = putUint16(b, foo)
b = putUint32(b, foo)
b = putUint16(b, foo)
b = putUint32(b, foo)
if _, err := w.cw.Write(buf[:]) {
  ...
}

and we don't have to see the offsets.

[rsc, 2012-02-27 15:49:21]

NOT LGTM

You should definitely avoid binary.Read if you care about
performance, but there is no reason to recreate
binary.LittleEndian.Uint16 etc.