Code review - Issue 6572043: code review 6572043: reflect: add ArrayOf, ChanOf, MapOf, SliceOfhttps://codereview.appspot.com/2012-11-13T18:06:28+00:00rietveld
Message from unknown
2012-09-25T00:34:15+00:00rscurn:md5:124a6c6a025b8e4f498f28fabb8f3d61
Message from unknown
2012-09-25T00:34:18+00:00rscurn:md5:f689a20c8ddc72187c0d9ac485511fc8
Message from unknown
2012-11-07T21:34:35+00:00rscurn:md5:2c42da8b943d000c9a176772a6d3f008
Message from rsc@golang.org
2012-11-07T21:34:49+00:00rscurn:md5:80ec19fdd5cb09db863d91e353cb21cd
Hello golang-dev@googlegroups.com,
I'd like you to review this change to
https://code.google.com/p/go/
Message from r@golang.org
2012-11-07T21:41:51+00:00rurn:md5:3c70d49370b77a1a025b69115bd7107b
haven't looked at the real code yet, but after a skim of the declarations i am confused by the relationship between runtimeType, rtype, commonType and whatever else there is that i've missed.
http://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/all_test.go
File src/pkg/reflect/all_test.go (right):
http://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/all_test.go#newcode2746
src/pkg/reflect/all_test.go:2746: t.Errorf("using constructed chan: have %q, %q, want %q, %q", s1, s2, "hello", "world")
s/using // for consistency
http://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/type.go
File src/pkg/reflect/type.go (right):
http://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/type.go#newcode238
src/pkg/reflect/type.go:238: // rtype is the common implementation of most values.
why the name change?
http://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/type.go#newcode980
src/pkg/reflect/type.go:980: func toCommonType(p *rtype) *rtype {
if the name changes (which i don't feel is necessary), this should change its name too.
since it's a no-op, though, why does it even exist?
http://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/type.go#newcode984
src/pkg/reflect/type.go:984: func toType(p *rtype) Type {
ditto, almost but not quite because of the nil interface thing
http://codereview.appspot.com/6572043/diff/4001/src/pkg/runtime/type.go
File src/pkg/runtime/type.go (right):
http://codereview.appspot.com/6572043/diff/4001/src/pkg/runtime/type.go#newcode17
src/pkg/runtime/type.go:17: // This should be rtype but saying commonType avoids having to update gdb.
is it rtype or commonType? what's going on?
Message from unknown
2012-11-07T21:42:22+00:00rscurn:md5:a84ddfb57730ed4b1bca41d4f692977d
Message from rsc@golang.org
2012-11-07T21:42:33+00:00rscurn:md5:2f27e3e5bdd67d6d82718bb52a843a9c
Hello golang-dev@googlegroups.com, r@golang.org (cc: golang-dev@googlegroups.com),
Please take another look.
Message from rsc@golang.org
2012-11-07T21:43:10+00:00rscurn:md5:18c055eac9beaace96ad2a3fe62f624f
Sorry, the PTAL is just because hg made me run gofmt and I thought the
original hadn't gone out. Nothing changed except the gofmting.
Message from remyoudompheng@gmail.com
2012-11-07T21:54:15+00:00remyoudomphengurn:md5:4a6daeddbde06617f19ca5d4bc12d594
http://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/all_test.go
File src/pkg/reflect/all_test.go (right):
http://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/all_test.go#newcode2717
src/pkg/reflect/all_test.go:2717: }
I would like to see something like:
vi := v.Interface()
if _, ok := vi.([10]T); !ok {
t.Errorf("constructed type %T != [10]T", vi)
}
http://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/all_test.go#newcode2731
src/pkg/reflect/all_test.go:2731: }
same test missing
http://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/all_test.go#newcode2747
src/pkg/reflect/all_test.go:2747: }
same test
http://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/all_test.go#newcode2763
src/pkg/reflect/all_test.go:2763:
same
http://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/type.go
File src/pkg/reflect/type.go (right):
http://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/type.go#newcode253
src/pkg/reflect/type.go:253: ptrToThis *rtype // type for pointer to this type, if used in binary or has methods
gofmt-ing was lost here
Message from rsc@golang.org
2012-11-07T21:54:21+00:00rscurn:md5:7ee92e171ae31922ad11685d15bc264f
https://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/all_test.go
File src/pkg/reflect/all_test.go (right):
https://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/all_test.go#newcode2746
src/pkg/reflect/all_test.go:2746: t.Errorf("using constructed chan: have %q, %q, want %q, %q", s1, s2, "hello", "world")
On 2012/11/07 21:41:51, r wrote:
> s/using // for consistency
Done.
https://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/type.go
File src/pkg/reflect/type.go (right):
https://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/type.go#newcode238
src/pkg/reflect/type.go:238: // rtype is the common implementation of most values.
On 2012/11/07 21:41:51, r wrote:
> why the name change?
There are two names being merged into one here. Before, there was both commonType and runtimeType. A runtimeType was a commonType preceded by an interface-valued header. The interface value described the commonType that followed. This was all nicely self-referential and worked (before we had kind we had to use type switches so the interface was important), but it was confusing to keep track of which type pointers were *commonType (pointing at this struct) vs *runtimeType (pointing one interface value before this struct), and crossing the boundary between reflect and runtime always had error-prone and awkward -2 or +2 to adjust the pointer two words. In essence commonType was the 'package reflect' type, and runtimeType was the 'package runtime' type.
In the cleaned up form, there is just one type in use, agreed upon between both the reflect package and the runtime. I named it rtype both because it now serves as the type pointer for both package runtime and package reflect, and also to make sure I was forced to look at all the existing uses of both names being replaced and adjust the code accordingly.
https://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/type.go#newcode980
src/pkg/reflect/type.go:980: func toCommonType(p *rtype) *rtype {
On 2012/11/07 21:41:51, r wrote:
> if the name changes (which i don't feel is necessary), this should change its
> name too.
>
> since it's a no-op, though, why does it even exist?
A dreg. Gone.
https://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/type.go#newcode984
src/pkg/reflect/type.go:984: func toType(p *rtype) Type {
On 2012/11/07 21:41:51, r wrote:
> ditto, almost but not quite because of the nil interface thing
Will be gone soon. This is another nice side effect of the cleanup.
https://codereview.appspot.com/6572043/diff/4001/src/pkg/runtime/type.go
File src/pkg/runtime/type.go (right):
https://codereview.appspot.com/6572043/diff/4001/src/pkg/runtime/type.go#newcode17
src/pkg/runtime/type.go:17: // This should be rtype but saying commonType avoids having to update gdb.
On 2012/11/07 21:41:51, r wrote:
> is it rtype or commonType? what's going on?
It is supposed to be rtype everywhere. Now that I know this code actually works - it was unclear for a long time - I will fix this. The gdb support knows this name so it is not as trivial to change.
Message from unknown
2012-11-07T22:04:30+00:00rscurn:md5:1a577bdb1c24522c87564d85edcfdbf2
Message from rsc@golang.org
2012-11-07T22:06:23+00:00rscurn:md5:04d76a1588c5ff83af16b70177f220e8
https://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/all_test.go
File src/pkg/reflect/all_test.go (right):
https://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/all_test.go#newcode2717
src/pkg/reflect/all_test.go:2717: }
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.
Message from unknown
2012-11-07T22:07:58+00:00rscurn:md5:cf6f136d8cb6ab5945160a931cd0437a
Message from r@golang.org
2012-11-07T22:38:47+00:00rurn:md5:76760df6aa0b294353d5363d90c8f7d1
http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go
File src/pkg/reflect/type.go (right):
http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newcode1017
src/pkg/reflect/type.go:1017: // of a *unsafe.Pointer.
s/a/an/ ?
http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newcode1249
src/pkg/reflect/type.go:1249: // typesByName returns the subslice of typelinks() with the given string.
typesByName returns the subslice of typelinks() whose elements have the given string representation.
http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newcode1276
src/pkg/reflect/type.go:1276: return typ[i:j]
looks like you get the empty slice (at the end of the typelinks slice) for no such string. should probably document that.
http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newcode1377
src/pkg/reflect/type.go:1377: ch.hash = typ.hash*16777619 ^ 'c'
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
}
http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newcode1386
src/pkg/reflect/type.go:1386: // MapOf returns the map type with the given key and value types.
s/value/element/
http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newcode1388
src/pkg/reflect/type.go:1388: // MapOf(k, v) represents map[int]string.
s/v/e/
http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newcode1391
src/pkg/reflect/type.go:1391: // not implement Go's == operator), MapOf panics.
s;$; TODO
http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newcode1470
src/pkg/reflect/type.go:1470: // ArrayOf panics.
it does?
Message from mirtchovski@gmail.com
2012-11-07T22:59:40+00:00aamurn:md5:920a1d6e8038664b9251dad9969f9da4
> where does this number come from?
http://primes.utm.edu/curios/page.php/16777619.html
Message from iant@golang.org
2012-11-07T23:36:53+00:00ianturn:md5:a164b5e3efe5c4dac7f2916526ec9678
https://codereview.appspot.com/6572043/diff/14001/src/cmd/ld/symtab.c
File src/cmd/ld/symtab.c (right):
https://codereview.appspot.com/6572043/diff/14001/src/cmd/ld/symtab.c#newcode389
src/cmd/ld/symtab.c:389: // s->hide = 1;
Why is s->hide = 1 commented out?
https://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/all_test.go
File src/pkg/reflect/all_test.go (right):
https://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/all_test.go#newcode2706
src/pkg/reflect/all_test.go:2706: func TestArrayOf(t *testing.T) {
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") }
https://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go
File src/pkg/reflect/type.go (right):
https://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newcode1252
src/pkg/reflect/type.go:1252: func typesByString(s string) []*rtype {
The comment says typesByName but the function is called typesByString.
https://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newcode1378
src/pkg/reflect/type.go:1378: ch.hash = typ.hash*16777619 ^ uint32(dir)
This code doesn't make sense as written. Do you want s/typ.hash/ch.hash/ ?
Message from unknown
2012-11-12T16:56:26+00:00rscurn:md5:84ccafada14654c74a03dffb0220154c
Message from rsc@golang.org
2012-11-12T16:56:33+00:00rscurn:md5:1d6d3eec54710ed7bd029960952c48a5
Hello r@golang.org, remyoudompheng@gmail.com, mirtchovski@gmail.com, iant@golang.org (cc: golang-dev@googlegroups.com),
Please take another look.
Message from r@golang.org
2012-11-13T00:31:25+00:00rurn:md5:28253378c69595762f11ae7cee630e1e
LGTM as far as i understand it.
https://codereview.appspot.com/6572043/diff/21001/src/cmd/gc/reflect.c
File src/cmd/gc/reflect.c (right):
https://codereview.appspot.com/6572043/diff/21001/src/cmd/gc/reflect.c#newcode635
src/cmd/gc/reflect.c:635: p = smprint("%-uT/%-T", t, t);
how much bigger is the .6?
https://codereview.appspot.com/6572043/diff/21001/src/cmd/ld/go.c
File src/cmd/ld/go.c (right):
https://codereview.appspot.com/6572043/diff/21001/src/cmd/ld/go.c#newcode743
src/cmd/ld/go.c:743: // keep each beginning with 'typelink.' if the symbol it points at is being kept.
keep each XXX beginning ...
https://codereview.appspot.com/6572043/diff/21001/src/cmd/ld/go.c#newcode780
src/cmd/ld/go.c:780:
tab
https://codereview.appspot.com/6572043/diff/21001/src/pkg/reflect/type.go
File src/pkg/reflect/type.go (right):
https://codereview.appspot.com/6572043/diff/21001/src/pkg/reflect/type.go#newcode1341
src/pkg/reflect/type.go:1341: // ChanOf returns the channel type with the given direction and and element type.
s/and and/and/
https://codereview.appspot.com/6572043/diff/21001/src/pkg/reflect/type.go#newcode1479
src/pkg/reflect/type.go:1479: // for the type. This may require significant work.
i thought i saw an export in one of the other files.
Message from iant@golang.org
2012-11-13T01:36:45+00:00ianturn:md5:b77b375a462e246e9835486f6ce540da
LGTM
https://codereview.appspot.com/6572043/diff/21001/src/cmd/gc/reflect.c
File src/cmd/gc/reflect.c (right):
https://codereview.appspot.com/6572043/diff/21001/src/cmd/gc/reflect.c#newcode525
src/cmd/gc/reflect.c:525: if(ot != 0)
Is this useful?
https://codereview.appspot.com/6572043/diff/21001/src/pkg/reflect/type.go
File src/pkg/reflect/type.go (left):
https://codereview.appspot.com/6572043/diff/21001/src/pkg/reflect/type.go#oldcode436
src/pkg/reflect/type.go:436: func (t *commonType) toType() Type {
The toType method is really convenient for gccgo, because that is where I canonicalize type pointers. If this patch takes it away, I'm going to have to change all the callers in the gccgo version of this code to call the canonicalization function directly. How would you feel about retaining this, or something similar--it doesn't have to be a method.
Message from rsc@golang.org
2012-11-13T17:59:16+00:00rscurn:md5:19af74a80fd88d5210e0bb7dab2f24c1
Sure, preserving toType is fine. I brought back a toType function and
documented why it exists.
Russ
Message from unknown
2012-11-13T18:06:11+00:00rscurn:md5:27679a7880341cbf792574706b35d80d
Message from rsc@golang.org
2012-11-13T18:06:28+00:00rscurn:md5:4c0b4fed10903b934ff054b233dd485e
*** 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/6572043
http://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/all_test.go
File src/pkg/reflect/all_test.go (right):
http://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/all_test.go#newcode2717
src/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#newcode2731
src/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#newcode2747
src/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#newcode2763
src/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.c
File src/cmd/ld/symtab.c (right):
http://codereview.appspot.com/6572043/diff/14001/src/cmd/ld/symtab.c#newcode389
src/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.go
File src/pkg/reflect/all_test.go (right):
http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/all_test.go#newcode2706
src/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.go
File src/pkg/reflect/type.go (right):
http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newcode1017
src/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#newcode1249
src/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#newcode1252
src/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#newcode1276
src/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#newcode1377
src/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#newcode1378
src/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#newcode1386
src/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#newcode1388
src/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#newcode1391
src/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#newcode1470
src/pkg/reflect/type.go:1470: // ArrayOf panics.
On 2012/11/07 22:38:47, r wrote:
> it does?
Done.