Adding compress/flate internal helper packages

[Matthew Dempsky]

Currently compress/flate has two copies of the Huffman decoder initialization code, one for package flate and one for the (package main) helper generator.  I want to deduplicate that code by moving it into a helper package compress/flate/internal/huffman.

That also requires moving the bit reversal code, so I'm proposing to move that code to compress/flate/internal/reverse.

Proposed CL for the above at https://go-review.googlesource.com/#/c/8977/.  However, Gobot has (thoroughly) reminded me that this requires also updating go/build/deps_test.go.

So I ask unto you, golang-dev, is it okay for me to add these new packages as dependencies to compress/flate?

Thanks

[Russ Cox]

Thanks for the suggestion, but I'd really rather not. Code duplication is not great but neither are the fragile hierarchies that result from trying to eliminate all duplication. We got away from those by not having subclassing and I don't want them to come back in a different form from use of internal.

Also I think you have found a bug in cmd/go, in that gen.go should not be able to import that internal package. Filed golang.org/issue/10478. So thanks for that. :-)

Russ

[Matthew Dempsky]

On Thu, Apr 16, 2015 at 7:11 AM, Russ Cox <r...@golang.org> wrote:

Thanks for the suggestion, but I'd really rather not. Code duplication is not great but neither are the fragile hierarchies that result from trying to eliminate all duplication. We got away from those by not having subclassing and I don't want them to come back in a different form from use of internal.

To be honest, I thought this would be a non-controversial proposal, so please indulge me in sharing the rest of my reasoning for proposing the refactoring:

  - Despite the (rather out of the way) comment warning that the code is duplicated into gen.go, it's diverged a few times (albeit in minor ways).

  - I'd like to try making some further cleanups/optimizations to the code, and having it in one place would make that (slightly) easier.

  - The DEFLATE algorithm is stable and unlikely to change.  The two extra packages I'm proposing (reverse and huffman) seem unlikely to have any fragility issues as they're very logically separated in RFC 1951 from the rest.

All that said, I respect your decision, so I'll continue working with the current code organization instead.

Cheers

[Nigel Tao]

On Thu, Apr 16, 2015 at 6:00 PM, 'Matthew Dempsky' via golang-dev
<golan...@googlegroups.com> wrote:
> Currently compress/flate has two copies of the Huffman decoder
> initialization code, one for package flate and one for the (package main)
> helper generator. I want to deduplicate that code by moving it into a
> helper package compress/flate/internal/huffman.

What we do in golang.org/x/text/cases is:

cases.go has this line:
//go:generate go run gen.go gen_trieval.go

gen_trieval.go is "package main".

gen.go copies gen_trieval.go to trieval.go, substitutes s/main/cases/
and adds a "This file was generated by go generate; DO NOT EDIT"
header.

Thus, there's one human-editable copy of the code (gen_trieval.go),
one generated copy of the code (trieval.go), and we are able to share
code between the package proper and its code generator, all in the one
directory and without an internal side package.

[Matthew Dempsky]

On Thu, Apr 16, 2015 at 4:45 PM, Nigel Tao <nige...@golang.org> wrote:

Thus, there's one human-editable copy of the code (gen_trieval.go),
one generated copy of the code (trieval.go), and we are able to share
code between the package proper and its code generator, all in the one
directory and without an internal side package.

Interesting. I still think an internal helper package would be cleaner, but this seems workable too.  I can look into implementing the same for compress/flate's huffman decoder if no one objects.