code review 139930044: runtime: convert type algorithms to Go

runtime: convert type algorithms to Go
Actually it mostly deletes code -- alg.print and alg.copy go away.
There was only one usage of alg.print for debug purposes.
Alg.copy is used in chan.goc, but Keith replaces them with
memcopy during conversion, so alg.copy is not needed as well.
Converting them would be significant amount of work
for no visible benefit.

[dvyukov, 2014-08-29 17:49:32]

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

I'd like you to review this change to
https://dvyukov%40google.com@code.google.com/p/go/

[crawshaw, 2014-08-29 17:56:09]

https://codereview.appspot.com/139930044/diff/100001/src/pkg/runtime/alg.go
File src/pkg/runtime/alg.go (right):

https://codereview.appspot.com/139930044/diff/100001/src/pkg/runtime/alg.go#n...
src/pkg/runtime/alg.go:51: {memhash, memequal},           // alg_MEM
You can specify an index in an array literal, which reduces the likelihood of
the comments being accidentally offset:

var algarray = [...]typeAlg{
        alg_MEM: {memhash, memequal},
        agl_MEM0: {memhash, memequal0},
        etc
}

[dvyukov, 2014-08-29 18:02:27]

https://codereview.appspot.com/139930044/diff/100001/src/pkg/runtime/alg.go
File src/pkg/runtime/alg.go (right):

https://codereview.appspot.com/139930044/diff/100001/src/pkg/runtime/alg.go#n...
src/pkg/runtime/alg.go:51: {memhash, memequal},           // alg_MEM
On 2014/08/29 17:56:09, crawshaw wrote:
> You can specify an index in an array literal, which reduces the likelihood of
> the comments being accidentally offset:
> 
> var algarray = [...]typeAlg{
>         alg_MEM: {memhash, memequal},
>         agl_MEM0: {memhash, memequal0},
>         etc
> }

Done
thanks

[crawshaw, 2014-08-29 19:28:45]

LGTM

but check with Russ about that print you removed from chan.goc.

[dvyukov, 2014-08-29 19:39:12]

On Fri, Aug 29, 2014 at 11:28 PM,  <crawshaw@golang.org> wrote:
> LGTM
>
> but check with Russ about that print you removed from chan.goc.

That printf is in C chansend and there is already Go chansend, so that
printf is already dead.

[rsc, 2014-08-29 19:48:38]

On Fri, Aug 29, 2014 at 3:28 PM, <crawshaw@golang.org> wrote:

> LGTM
> but check with Russ about that print you removed from chan.goc.
>

Anyone is welcome to review this CLs, but in general all this conversion
stuff is subtle enough that you should (for any conversion CL) wait for an
LGTM from me or Keith before submitting. Thanks.

Russ

[khr, 2014-08-29 21:09:09]

LGTM.

https://codereview.appspot.com/139930044/diff/120001/src/pkg/runtime/alg.go
File src/pkg/runtime/alg.go (right):

https://codereview.appspot.com/139930044/diff/120001/src/pkg/runtime/alg.go#n...
src/pkg/runtime/alg.go:139: return f64hash(unsafe.Pointer(&x[1]), 4,
f64hash(unsafe.Pointer(&x[0]), 4, h))
s/4/8/ while you are here.

Not that it matters.

[dvyukov, 2014-08-30 04:31:02]

https://codereview.appspot.com/139930044/diff/120001/src/pkg/runtime/alg.go
File src/pkg/runtime/alg.go (right):

https://codereview.appspot.com/139930044/diff/120001/src/pkg/runtime/alg.go#n...
src/pkg/runtime/alg.go:139: return f64hash(unsafe.Pointer(&x[1]), 4,
f64hash(unsafe.Pointer(&x[0]), 4, h))
On 2014/08/29 21:09:09, khr wrote:
> s/4/8/ while you are here.
> 
> Not that it matters.

Done.

[dvyukov, 2014-08-30 04:41:03]

*** Submitted as https://code.google.com/p/go/source/detail?r=7b4959d78977 ***

runtime: convert type algorithms to Go
Actually it mostly deletes code -- alg.print and alg.copy go away.
There was only one usage of alg.print for debug purposes.
Alg.copy is used in chan.goc, but Keith replaces them with
memcopy during conversion, so alg.copy is not needed as well.
Converting them would be significant amount of work
for no visible benefit.

LGTM=crawshaw, rsc, khr
R=golang-codereviews, crawshaw, khr
CC=golang-codereviews, rsc
https://codereview.appspot.com/139930044

[gobot, 2014-08-30 04:42:03]

This CL appears to have broken the windows-amd64-perf builder.
See http://build.golang.org/log/a04b8d846d357f0b681e8fb0e414c5563312137d