code review 5448065: Add a []byte argument to hash.Hash to allow an allocati...

Add a []byte argument to hash.Hash to allow an allocation to be saved.

This is the result of running `gofix -r hashsum` over the tree, changing
the hash function implementations by hand and then fixing a couple of
instances where gofix didn't catch something.

The changed implementations are as simple as possible while still
working: I'm not trying to optimise in this CL.

[agl1, 2011-11-30 21:03:10]

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

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

[rsc, 2011-11-30 21:19:40]

LGTM

For my own curiosity, what didn't gofix fix?

http://codereview.appspot.com/5448065/diff/2002/src/pkg/hash/hash.go
File src/pkg/hash/hash.go (right):

http://codereview.appspot.com/5448065/diff/2002/src/pkg/hash/hash.go#newcode16
src/pkg/hash/hash.go:16: // Sum appends the current hash in the same manner as
append(), without
s/in/dst/

// Sum returns the result of appending the current hash to dst, without ...

[cw, 2011-12-01 06:14:21]

What about having the Sum() only be valid immediately after it's called and
embedding the allocation in the hash?

[rog, 2011-12-01 09:31:35]

that would mean that concurrent use of Sum would be inherently unsafe.

On 1 December 2011 06:14,  <cw@f00f.org> wrote:
> What about having the Sum() only be valid immediately after it's called
> and embedding the allocation in the hash?
>
> http://codereview.appspot.com/5448065/

[agl1, 2011-12-01 16:01:27]

On Thu, Dec 1, 2011 at 1:14 AM,  <cw@f00f.org> wrote:
> What about having the Sum() only be valid immediately after it's called
> and embedding the allocation in the hash?

I considered this but, in many cases, returning a new []byte is
actually what the caller wants, so all those callers would have to
call append() to make a copy. It's also a sharp edge, not to mention
that changing the semantics without changing the API is going to cause
some people problems.

For the cases where the caller is hashing a lot, Sum([]byte) and a Sum
that returns an internal buffer is about the same. So, on balance, I
went with the Sum([]byte).

But I'm not committed either way if people prefer the returning an
internal buffer.


Cheers

AGL

[cw, 2011-12-01 16:05:50]

> But I'm not committed either way if people prefer the returning an
> internal buffer.

i wanted this some time ago when doing a hand tuned md5 (porting
someone elses code) and an internal buffer seemed to match that better
then but i didn't consider passing []byte

i suspect the performance difference won't be that noticable (md5 even
when tuned is going to be quite slow and a 16-byte copy shouldn't be
that bad)

[rsc, 2011-12-01 16:47:54]

I thought some more about this on the way in.
I think the appending really makes more sense.
It gives the caller control over memory in a way
that the shared buffer does not, for all the reasons
that Adam outlined.  The only other API I can think
of that behaves like Chris's suggestion is bufio's
ReadSlice and that has confused nearly everyone
who looked at it.

Let's stick with Adam's appending API.

Russ

[agl1, 2011-12-01 17:19:15]

On Wed, Nov 30, 2011 at 4:19 PM,  <rsc@golang.org> wrote:
> For my own curiosity, what didn't gofix fix?

It missed the crypto.MD5.New stuff, because I hadn't written that in
gofix at the time that I ran it. I believe that it would have caught
that now.

Other than that, it only missed cases where I had written hash.Hash
implementations and forgotten about them. (i.e. the truncating hash in
exp/ssh.) However, those require more intervention than an automated
tool is going to manage.

Will land things now.


Cheers

AGL

[agl1, 2011-12-01 17:35:45]

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

Add a []byte argument to hash.Hash to allow an allocation to be saved.

This is the result of running `gofix -r hashsum` over the tree, changing
the hash function implementations by hand and then fixing a couple of
instances where gofix didn't catch something.

The changed implementations are as simple as possible while still
working: I'm not trying to optimise in this CL.

R=rsc, cw, rogpeppe
CC=golang-dev
http://codereview.appspot.com/5448065