*** Submitted as
http://code.google.com/p/go/source/detail?r=2e2a1d8184e6 ***
reflect: add ArrayOf, ChanOf, MapOf, SliceOf
In order to add these, we need to be able to find references
to such types that already exist in the binary. To do that, introduce
a new linker section holding a list of the types corresponding to
arrays, chans, maps, and slices.
To offset the storage cost of this list, and to simplify the code,
remove the interface{} header from the representation of a
runtime type. It was used in early versions of the code but was
made obsolete by the kind field: a switch on kind is more efficient
than a type switch.
In the godoc binary, removing the interface{} header cuts two
words from each of about 10,000 types. Adding back the list of pointers
to array, chan, map, and slice types reintroduces one word for
each of about 500 types. On a 64-bit machine, then, this CL *removes*
a net 156 kB of read-only data from the binary.
This CL does not include the needed support for precise garbage
collection. I have created issue 4375 to track that.
This CL also does not set the 'algorithm' - specifically the equality
and copy functions - for a new array correctly, so I have unexported
ArrayOf for now. That is also part of issue 4375.
Fixes issue 2339.
R=r, remyoudompheng, mirtchovski, iant
CC=golang-dev
http://codereview.appspot.com/6572043http://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/all_test.goFile src/pkg/reflect/all_test.go (right):
http://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/all_test.go#newcode2717src/pkg/reflect/all_test.go:2717: }
On 2012/11/07 22:06:23, rsc wrote:On 2012/11/07 21:54:15, remyoudompheng wrote:
I would like to see something like:
vi := v.Interface()
if _, ok := vi.([10]T); !ok {
t.Errorf("constructed type %T != [10]T", vi)
}
The tricky part is that if I add that test I lose what the above is testing,
namely that the newly constructed type ends up working. If the binary mentions
[10]T explicitly, reflect won't have to synthesize [10]T. However, I can add a
test for something different, like [5]T.
Done.
http://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/all_test.go#newcode2731src/pkg/reflect/all_test.go:2731: }
On 2012/11/07 21:54:15, remyoudompheng wrote:
same test missing
Done.
http://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/all_test.go#newcode2747src/pkg/reflect/all_test.go:2747: }
On 2012/11/07 21:54:15, remyoudompheng wrote:
same test
Done.
http://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/all_test.go#newcode2763src/pkg/reflect/all_test.go:2763:
On 2012/11/07 21:54:15, remyoudompheng wrote:
same
Done.
http://codereview.appspot.com/6572043/diff/14001/src/cmd/ld/symtab.cFile src/cmd/ld/symtab.c (right):
http://codereview.appspot.com/6572043/diff/14001/src/cmd/ld/symtab.c#newcode389src/cmd/ld/symtab.c:389: // s->hide = 1;
On 2012/11/07 23:36:53, iant wrote:
Why is s->hide = 1 commented out?
Probably a debugging dreg; fixed.
http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/all_test.goFile src/pkg/reflect/all_test.go (right):
http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/all_test.go#newcode2706src/pkg/reflect/all_test.go:2706: func TestArrayOf(t *testing.T) {
On 2012/11/07 23:36:53, iant wrote:
I think you also need some tests that these functions return the same type as
the type in the program.
type F float
at := ArrayOf(10, TypeOf(F(1)))
if at != TypeOf([10]F) { panic("different type") }
Done.
http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.goFile src/pkg/reflect/type.go (right):
http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newcode1017src/pkg/reflect/type.go:1017: // of a *unsafe.Pointer.
On 2012/11/07 22:38:47, r wrote:
s/a/an/ ?
Done.
http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newcode1249src/pkg/reflect/type.go:1249: // typesByName returns the subslice of
typelinks() with the given string.
On 2012/11/07 22:38:47, r wrote:
typesByName returns the subslice of typelinks() whose elements have the given
string representation.
Done.
http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newcode1252src/pkg/reflect/type.go:1252: func typesByString(s string) []*rtype {
On 2012/11/07 23:36:53, iant wrote:
The comment says typesByName but the function is called typesByString.
Done.
http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newcode1276src/pkg/reflect/type.go:1276: return typ[i:j]
On 2012/11/07 22:38:47, r wrote:
looks like you get the empty slice (at the end of the typelinks slice) for no
such string. should probably document that.
Done.
http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newcode1377src/pkg/reflect/type.go:1377: ch.hash = typ.hash*16777619 ^ 'c'
On 2012/11/07 22:38:47, r wrote:
where does this number come from? at least give it a name - it appears
everywhere. regardless, i think you want a variadic function that
constructs the
hash value, so the number appears in only one place. this isn't quite it, i
don't think, but something like it would be nice:
func newTypeHash(x ...uintptr) (h uintptr) {
for u := range x {
h = h*16777619 ^ x
}
return
}
Done.
http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newcode1378src/pkg/reflect/type.go:1378: ch.hash = typ.hash*16777619 ^ uint32(dir)
On 2012/11/07 23:36:53, iant wrote:
This code doesn't make sense as written. Do you want
s/typ.hash/ch.hash/ ?
Done.
http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newcode1386src/pkg/reflect/type.go:1386: // MapOf returns the map type with the
given key and value types.
On 2012/11/07 22:38:47, r wrote:
s/value/element/
Done.
http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newcode1388src/pkg/reflect/type.go:1388: // MapOf(k, v) represents map[int]string.
On 2012/11/07 22:38:47, r wrote:
s/v/e/
Done.
http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newcode1391src/pkg/reflect/type.go:1391: // not implement Go's == operator), MapOf
panics.
On 2012/11/07 22:38:47, r wrote:
s;$; TODO
Done.
http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newcode1470src/pkg/reflect/type.go:1470: // ArrayOf panics.
On 2012/11/07 22:38:47, r wrote:
it does?
Done.
http://codereview.appspot.com/6572043/
