code review 5450073: gzip: Convert between Latin-1 and Unicode

gzip: Convert between Latin-1 and Unicode

I realize I didn't send the tests in last time.  Anyway, I added
a test that knows too much about the package's internal structure,
and I'm not sure whether it's the right thing to do.

Vadik.

[vadik, 2011-12-02 20:36:23]

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, 2011-12-02 21:37:35]

http://codereview.appspot.com/5450073/diff/2001/src/pkg/compress/gzip/gunzip.go
File src/pkg/compress/gzip/gunzip.go (right):

http://codereview.appspot.com/5450073/diff/2001/src/pkg/compress/gzip/gunzip....
src/pkg/compress/gzip/gunzip.go:111: s += string(v)
repeatedly appending to and replacing a string like this is not cheap

[rsc, 2011-12-02 21:45:48]

Thanks for looking into fixing this.

A better conversion strategy would be to first check
whether there are any non-ASCII characters in the
string before bothering to do a conversion.  Then, if
there are any, an easy way to do the conversion
is to make a []rune of the same length as the string,
copy each byte in as a rune, and then convert the []rune
back to string using a conversion: string(runes).

Also, if we fix the code but don't add a test that it's
fixed, it will inevitably get broken again, so please
add a test of both new pieces of code.

Thanks.
Russ

[vadik, 2011-12-02 23:40:29]

On 2011/12/02 21:45:48, rsc wrote:
> Thanks for looking into fixing this.
> 
> A better conversion strategy would be to first check
> whether there are any non-ASCII characters in the
> string before bothering to do a conversion.  Then, if
> there are any, an easy way to do the conversion
> is to make a []rune of the same length as the string,
> copy each byte in as a rune, and then convert the []rune
> back to string using a conversion: string(runes).
> 
> Also, if we fix the code but don't add a test that it's
> fixed, it will inevitably get broken again, so please
> add a test of both new pieces of code.
> 
> Thanks.
> Russ

Thanks for the explanation, will do.

Vadik.

[vadik, 2011-12-03 00:26:52]

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

Please take another look.

[vadik, 2011-12-03 00:29:43]

On 2011/12/03 00:26:52, vadik wrote:
> Hello mailto:golang-dev@googlegroups.com, mailto:bradfitz@golang.org,
mailto:rsc@golang.org (cc:
> mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

I'm quite new to hg, so forgive me for sending extra mail.

In the latest patch, I check whether conversion is needed and use []rune.  I
changed the test suite to send some Latin-1 characters in the comment field --
should I make more extensive tests (e.g., for failing when we have runes >0xff)?

Vadik.

[peterGo, 2011-12-05 15:37:00]

I prefer simple, direct, easy-to-read, and reasonably efficient solutions. When
I benchmarked your complicated solution, it often took longer, sometimes a lot
longer, than both the original code and my solution.

Don't do things unnecessarily: loops, allocations, etc. Keep things simple.

[peterGo, 2011-12-05 15:48:52]

For example,

func (z *Decompressor) readString() (string, error) {
	// GZIP (RFC 1952) specifies that strings are 
	// NUL-terminated ISO 8859-1 (Latin-1).
	for i := 0; ; {
		iso, err := z.r.ReadByte()
		if err != nil {
			return "", err
		}
		if iso == 0 {
			return string(z.buf[0:i]), nil
		}
		if i > len(z.buf)-utf8.UTFMax {
			return "", HeaderError
		}
		if iso < utf8.RuneSelf {
			z.buf[i] = iso
			i++
		} else {
			i += utf8.EncodeRune(z.buf[i:], rune(iso))
		}
	}
	panic("not reached")
}

Peter

On 2011/12/05 15:37:00, peterGo wrote:
> I prefer simple, direct, easy-to-read, and reasonably efficient solutions.
When
> I benchmarked your complicated solution, it often took longer, sometimes a lot
> longer, than both the original code and my solution.
> 
> Don't do things unnecessarily: loops, allocations, etc. Keep things simple.

[rsc, 2011-12-07 17:24:11]

http://codereview.appspot.com/5450073/diff/7001/src/pkg/compress/gzip/gunzip.go
File src/pkg/compress/gzip/gunzip.go (right):

http://codereview.appspot.com/5450073/diff/7001/src/pkg/compress/gzip/gunzip....
src/pkg/compress/gzip/gunzip.go:113: if needconv {
This code is great.  I don't believe you need the whole-hog
version that Peter suggested.  However, please do add a test.

[vadik, 2011-12-09 11:21:40]

Hello bradfitz@golang.org, rsc@golang.org, go.peter.90@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[vadik, 2011-12-09 11:25:36]

Hello bradfitz@golang.org, rsc@golang.org, go.peter.90@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[vadik, 2011-12-09 11:27:41]

On 2011/12/09 11:25:36, vadik wrote:
> Hello mailto:bradfitz@golang.org, mailto:rsc@golang.org,
mailto:go.peter.90@gmail.com (cc:
> mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

I realize I didn't send the tests in last time.  Anyway, I added
a test that knows too much about the package's internal structure,
and I'm not sure whether it's the right thing to do.

Vadik.

[rsc, 2011-12-14 21:28:43]

LGTM

[rsc, 2011-12-14 22:17:43]

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

gzip: Convert between Latin-1 and Unicode

I realize I didn't send the tests in last time.  Anyway, I added
a test that knows too much about the package's internal structure,
and I'm not sure whether it's the right thing to do.

Vadik.

R=bradfitz, rsc, go.peter.90
CC=golang-dev
http://codereview.appspot.com/5450073

Committer: Russ Cox <rsc@golang.org>