code review 4182081: compress/lzw: implement a decoder.

compress/lzw: implement a decoder.

[nigeltao, 2011-02-20 16:19:33]

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

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

[rsc, 2011-02-22 21:13:09]

LGTM

http://codereview.appspot.com/4182081/diff/10006/src/pkg/compress/lzw/reader.go
File src/pkg/compress/lzw/reader.go (right):

http://codereview.appspot.com/4182081/diff/10006/src/pkg/compress/lzw/reader....
src/pkg/compress/lzw/reader.go:36: func nextCodeLSB(d *decoder) (uint16,
os.Error) {
you can make this func (d *decoder) lsb() (uint16, os.Error)
and refer to it below as (*decoder).lsb.
that will avoid any clash with encoder functions later.

or you can leave it as is.

also, whichever of these is untested is probably wrong.
i expected one of them to need a byte-reverse lookup table
like in compress/flate.

http://codereview.appspot.com/4182081/diff/10006/src/pkg/compress/lzw/reader....
src/pkg/compress/lzw/reader.go:176: func NewReader(r io.Reader, packingOrder
string, litWidth int) io.ReadCloser {
define a type for packing order

type Order int
const (
    LSB Order = iota
    MSB
)

http://codereview.appspot.com/4182081/diff/10006/src/pkg/compress/lzw/reader....
src/pkg/compress/lzw/reader.go:193: pw.CloseWithError(decode(pw,
bufio.NewReader(r), nextCode, uint(litWidth)))
should do what flate does: sniff to see if it already has ReadByte
to avoid double-buffering

[nigeltao, 2011-02-23 07:31:48]

http://codereview.appspot.com/4182081/diff/10006/src/pkg/compress/lzw/reader.go
File src/pkg/compress/lzw/reader.go (right):

http://codereview.appspot.com/4182081/diff/10006/src/pkg/compress/lzw/reader....
src/pkg/compress/lzw/reader.go:36: func nextCodeLSB(d *decoder) (uint16,
os.Error) {
On 2011/02/22 21:13:09, rsc wrote:
> you can make this func (d *decoder) lsb() (uint16, os.Error)
> and refer to it below as (*decoder).lsb.
> that will avoid any clash with encoder functions later.

Done.


> also, whichever of these is untested is probably wrong.
> i expected one of them to need a byte-reverse lookup table
> like in compress/flate.

I suspect that LSB/MSB means something different than what you think it means.

I've added a test case for a PDF LZW stream that I found on the internet.

http://codereview.appspot.com/4182081/diff/10006/src/pkg/compress/lzw/reader....
src/pkg/compress/lzw/reader.go:176: func NewReader(r io.Reader, packingOrder
string, litWidth int) io.ReadCloser {
On 2011/02/22 21:13:09, rsc wrote:
> define a type for packing order
> 
> type Order int
> const (
>     LSB Order = iota
>     MSB
> )
> 

Done.

http://codereview.appspot.com/4182081/diff/10006/src/pkg/compress/lzw/reader....
src/pkg/compress/lzw/reader.go:193: pw.CloseWithError(decode(pw,
bufio.NewReader(r), nextCode, uint(litWidth)))
On 2011/02/22 21:13:09, rsc wrote:
> should do what flate does: sniff to see if it already has ReadByte
> to avoid double-buffering

Done.

[nigeltao, 2011-02-23 09:52:54]

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

compress/lzw: implement a decoder.

R=rsc
CC=bsiegert, golang-dev, mpl
http://codereview.appspot.com/4182081