Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(154)

Issue 5699097: code review 5699097: archive/zip: use encoding/binary again, add readBuf helper (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 1 month ago by adg
Modified:
12 years, 1 month ago
Reviewers:
CC:
golang-dev, bradfitzgoog
Visibility:
Public.

Description

archive/zip: use encoding/binary again, add readBuf helper

Patch Set 1 #

Total comments: 6

Patch Set 2 : diff -r ec85c3a91aab https://go.googlecode.com/hg #

Unified diffs Side-by-side diffs Delta from patch set Stats (+81 lines, -67 lines) Patch
M src/pkg/archive/zip/reader.go View 1 7 chunks +78 lines, -58 lines 0 comments Download
M src/pkg/archive/zip/writer.go View 2 chunks +3 lines, -9 lines 0 comments Download

Messages

Total messages: 3
adg
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg
12 years, 1 month ago (2012-02-27 22:29:45 UTC) #1
bradfitzgoog
LGTM http://codereview.appspot.com/5699097/diff/1/src/pkg/archive/zip/reader.go File src/pkg/archive/zip/reader.go (right): http://codereview.appspot.com/5699097/diff/1/src/pkg/archive/zip/reader.go#newcode300 src/pkg/archive/zip/reader.go:300: return nil, errors.New("invalid comment length") "zip: ..." http://codereview.appspot.com/5699097/diff/1/src/pkg/archive/zip/reader.go#newcode323 ...
12 years, 1 month ago (2012-02-27 22:34:21 UTC) #2
adg
12 years, 1 month ago (2012-02-27 22:41:32 UTC) #3
*** Submitted as http://code.google.com/p/go/source/detail?r=4723d6672df6 ***

archive/zip: use encoding/binary again, add readBuf helper

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

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

http://codereview.appspot.com/5699097/diff/1/src/pkg/archive/zip/reader.go#ne...
src/pkg/archive/zip/reader.go:300: return nil, errors.New("invalid comment
length")
On 2012/02/27 22:34:21, bradfitzgoog wrote:
> "zip: ..."

Done.

http://codereview.appspot.com/5699097/diff/1/src/pkg/archive/zip/reader.go#ne...
src/pkg/archive/zip/reader.go:323: c := *b
On 2012/02/27 22:34:21, bradfitzgoog wrote:
> c is a bit of a weird variable name.  I'd use a temporary named v instead.  I
> think this reads easier:
> 
> v := binary.LittleEndian.Uint16(*b)
> *b = (*b)[2:]
> return v

Done.

http://codereview.appspot.com/5699097/diff/1/src/pkg/archive/zip/reader.go#ne...
src/pkg/archive/zip/reader.go:329: c := *b
On 2012/02/27 22:34:21, bradfitzgoog wrote:
> likewise

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b