code review 112700043: runtime: move constants from map header to map type

runtime: move constants from map header to map type

A good cleanup anyway, and it makes some room for an additional
field needed for issue 8412.

Update issue 8412

[khr, 2014-07-25 18:33:53]

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

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

[iant, 2014-08-01 13:50:40]

https://codereview.appspot.com/112700043/diff/60001/src/cmd/gc/reflect.c
File src/cmd/gc/reflect.c (right):

https://codereview.appspot.com/112700043/diff/60001/src/cmd/gc/reflect.c#newc...
src/cmd/gc/reflect.c:1116: ot = duint16(s, ot, mapbucket(t)->width);
Why bother with this field?  It's already available via the bucket type field.

https://codereview.appspot.com/112700043/diff/60001/src/pkg/reflect/type.go
File src/pkg/reflect/type.go (right):

https://codereview.appspot.com/112700043/diff/60001/src/pkg/reflect/type.go#n...
src/pkg/reflect/type.go:332: indirectkey   uint8  // store ptr to key instead of
key itself
Seems to me that indirectkey and indirectvalue should be type bool.

[khr1, 2014-08-01 20:39:24]

On Fri, Aug 1, 2014 at 6:50 AM, <iant@golang.org> wrote:

>
> https://codereview.appspot.com/112700043/diff/60001/src/cmd/gc/reflect.c
> File src/cmd/gc/reflect.c (right):
>
> https://codereview.appspot.com/112700043/diff/60001/src/
> cmd/gc/reflect.c#newcode1116
> src/cmd/gc/reflect.c:1116: ot = duint16(s, ot, mapbucket(t)->width);
> Why bother with this field?  It's already available via the bucket type
> field.
>
>
It's one less indirection to get to it.  Due to the magic of alignment it
doesn't use any space (at least on 64-bit platforms).


> https://codereview.appspot.com/112700043/diff/60001/src/
> pkg/reflect/type.go
> File src/pkg/reflect/type.go (right):
>
> https://codereview.appspot.com/112700043/diff/60001/src/
> pkg/reflect/type.go#newcode332
> src/pkg/reflect/type.go:332: indirectkey   uint8  // store ptr to key
> instead of key itself
> Seems to me that indirectkey and indirectvalue should be type bool.
>
>
In an ideal world, yes.  Unfortunately, these fields are defined in C where
bool is just a typedef of uint8 so the C->Go struct converter can't tell
the difference.  So right now the Go generated structure (in
zruntime_defs_*.go) uses uint8s.  I wanted the reflect type to match that
type.  Although I guess it's not strictly necessary.

I hope to one day soon start having the truth about runtime structures
defined in Go and autotranslated back to C.  At that point we can fix this.


> https://codereview.appspot.com/112700043/
>

[iant, 2014-08-01 21:25:10]

LGTM

[khr, 2014-08-01 21:39:01]

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

runtime: move constants from map header to map type

A good cleanup anyway, and it makes some room for an additional
field needed for issue 8412.

Update issue 8412

LGTM=iant
R=iant, khr
CC=golang-codereviews
https://codereview.appspot.com/112700043

[gobot, 2014-08-01 22:06:53]

This CL appears to have broken the windows-386 builder.
See http://build.golang.org/log/da012bcaccbc9514fdc28df2917425b951e3d415

[khr1, 2014-08-01 22:11:09]

Not related.



On Fri, Aug 1, 2014 at 3:06 PM, <gobot@golang.org> wrote:

> This CL appears to have broken the windows-386 builder.
> See http://build.golang.org/log/da012bcaccbc9514fdc28df2917425b951e3d415
>
> https://codereview.appspot.com/112700043/
>