Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(705)

Issue 4281055: code review 4281055: reflect: new Type and Value definitions (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
13 years, 7 months ago by rsc
Modified:
13 years, 6 months ago
Reviewers:
CC:
gri, iant, niemeyer, r, rog, gustavo_niemeyer.net, r2, golang-dev
Visibility:
Public.

Description

reflect: new Type and Value definitions Type is now an interface that implements all the possible type methods. Instead of a type switch on a reflect.Type t, switch on t.Kind(). If a method is invoked on the wrong kind of type (for example, calling t.Field(0) when t.Kind() != Struct), the call panics. There is one method renaming: t.(*ChanType).Dir() is now t.ChanDir(). Value is now a struct value that implements all the possible value methods. Instead of a type switch on a reflect.Value v, switch on v.Kind(). If a method is invoked on the wrong kind of value (for example, calling t.Recv() when t.Kind() != Chan), the call panics. Since Value is now a struct, not an interface, its zero value cannot be compared to nil. Instead of v != nil, use v.IsValid(). Instead of other uses of nil as a Value, use Value{}, the zero value. Many methods have been renamed, most due to signature conflicts: OLD NEW v.(*ArrayValue).Elem v.Index v.(*BoolValue).Get v.Bool v.(*BoolValue).Set v.SetBool v.(*ChanType).Dir v.ChanDir v.(*ChanValue).Get v.Pointer v.(*ComplexValue).Get v.Complex v.(*ComplexValue).Overflow v.OverflowComplex v.(*ComplexValue).Set v.SetComplex v.(*FloatValue).Get v.Float v.(*FloatValue).Overflow v.OverflowFloat v.(*FloatValue).Set v.SetFloat v.(*FuncValue).Get v.Pointer v.(*InterfaceValue).Get v.InterfaceData v.(*IntValue).Get v.Int v.(*IntValue).Overflow v.OverflowInt v.(*IntValue).Set v.SetInt v.(*MapValue).Elem v.MapIndex v.(*MapValue).Get v.Pointer v.(*MapValue).Keys v.MapKeys v.(*MapValue).SetElem v.SetMapIndex v.(*PtrValue).Get v.Pointer v.(*SliceValue).Elem v.Index v.(*SliceValue).Get v.Pointer v.(*StringValue).Get v.String v.(*StringValue).Set v.SetString v.(*UintValue).Get v.Uint v.(*UintValue).Overflow v.OverflowUint v.(*UintValue).Set v.SetUint v.(*UnsafePointerValue).Get v.Pointer v.(*UnsafePointerValue).Set v.SetPointer Part of the motivation for this change is to enable a more efficient implementation of Value, one that does not allocate memory during most operations. To reduce the size of the CL, this CL's implementation is a wrapper around the old API. Later CLs will make the implementation more efficient without changing the API. Other CLs to be submitted at the same time as this one add support for this change to gofix (4343047) and update the Go source tree (4353043).

Patch Set 1 #

Patch Set 2 : diff -r 424b74bdf29e https://go.googlecode.com/hg/ #

Total comments: 2

Patch Set 3 : diff -r 82050f8e7881 https://go.googlecode.com/hg/ #

Patch Set 4 : diff -r 82050f8e7881 https://go.googlecode.com/hg/ #

Total comments: 44

Patch Set 5 : diff -r e1efe298340c https://go.googlecode.com/hg #

Total comments: 2

Patch Set 6 : diff -r 1d152ae12dfc https://go.googlecode.com/hg #

Total comments: 1

Patch Set 7 : diff -r ce9962e29e4b https://go.googlecode.com/hg #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1436 lines, -887 lines) Patch
M src/pkg/reflect/all_test.go View 1 2 3 4 5 43 chunks +176 lines, -217 lines 0 comments Download
M src/pkg/reflect/deepequal.go View 1 2 2 chunks +24 lines, -34 lines 0 comments Download
M src/pkg/reflect/tostring_test.go View 1 2 4 chunks +27 lines, -27 lines 0 comments Download
M src/pkg/reflect/type.go View 1 2 3 4 5 19 chunks +421 lines, -334 lines 0 comments Download
M src/pkg/reflect/value.go View 1 2 3 4 5 6 52 chunks +788 lines, -275 lines 0 comments Download

Messages

Total messages: 36
r
I think this is a variant of an earlier design, the variance being that all ...
13 years, 7 months ago (2011-03-19 06:21:04 UTC) #1
rsc
On Sat, Mar 19, 2011 at 02:21, <r@golang.org> wrote: > I think this is a ...
13 years, 7 months ago (2011-03-19 06:54:31 UTC) #2
niemeyer
While the amount of clutter removed feels very good, part of me is wondering about ...
13 years, 7 months ago (2011-03-19 16:23:10 UTC) #3
rsc
You're right but I think reflect is always a bit different. It exists to do ...
13 years, 7 months ago (2011-03-19 16:47:09 UTC) #4
iant
With this approach, why retain the distinction between the runtime type structures and the reflect ...
13 years, 7 months ago (2011-03-20 04:30:39 UTC) #5
rsc
On Sun, Mar 20, 2011 at 00:30, <iant@golang.org> wrote: > With this approach, why retain ...
13 years, 7 months ago (2011-03-20 04:40:47 UTC) #6
gri
FYI http://codereview.appspot.com/4281055/diff/1001/src/pkg/reflect/type.go File src/pkg/reflect/type.go (right): http://codereview.appspot.com/4281055/diff/1001/src/pkg/reflect/type.go#newcode74 src/pkg/reflect/type.go:74: // a value of the given type; it ...
13 years, 7 months ago (2011-03-21 18:56:29 UTC) #7
rsc
Hello golang-dev, gri, iant, niemeyer, r (cc: golang-dev@googlegroups.com), I'd like you to review this change ...
13 years, 6 months ago (2011-04-02 01:02:14 UTC) #8
rsc
reflect: new Type and Value definitions Type is now an interface that implements all the ...
13 years, 6 months ago (2011-04-02 01:08:03 UTC) #9
rsc
My plan is to submit these changes (and the related CLs, which I'll send out ...
13 years, 6 months ago (2011-04-02 01:09:17 UTC) #10
rsc
For what it's worth, these are the diffs necessary to the tree to convert to ...
13 years, 6 months ago (2011-04-02 01:13:56 UTC) #11
r
be interesting to know if this makes a measurable performance difference on, say, the JSON ...
13 years, 6 months ago (2011-04-02 18:45:33 UTC) #12
klownkeeper_gmail.com
So was taruti right or wrong for saying DotDotDot instead of HasDotDotDot and how much ...
13 years, 6 months ago (2011-04-03 04:28:29 UTC) #13
rsc
On Sun, Apr 3, 2011 at 00:28, Keith Clark <klownkeeper@gmail.com> wrote: > So was taruti ...
13 years, 6 months ago (2011-04-03 15:18:17 UTC) #14
niemeyer
Feels like going in a good direction. Some comments follow. http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/all_test.go File src/pkg/reflect/all_test.go (right): http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/all_test.go#newcode217 ...
13 years, 6 months ago (2011-04-04 13:52:34 UTC) #15
niemeyer
Also, just as a reminder, the following issue was targeted at the next redesign, so ...
13 years, 6 months ago (2011-04-04 16:23:46 UTC) #16
rog
http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go File src/pkg/reflect/value.go (right): http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#newcode58 src/pkg/reflect/value.go:58: Internal valueInterface > why is Internal exported? we don't ...
13 years, 6 months ago (2011-04-04 17:06:49 UTC) #17
rsc
On Sat, Apr 2, 2011 at 14:45, <r@golang.org> wrote: > be interesting to know if ...
13 years, 6 months ago (2011-04-04 20:14:32 UTC) #18
r
LGTM http://codereview.appspot.com/4281055/diff/18002/src/pkg/reflect/type.go File src/pkg/reflect/type.go (right): http://codereview.appspot.com/4281055/diff/18002/src/pkg/reflect/type.go#newcode90 src/pkg/reflect/type.go:90: // sized or unsized Int, Uint, Float, or ...
13 years, 6 months ago (2011-04-04 23:26:53 UTC) #19
rsc
done will submit with 4285053 and 4343047 once they are ready
13 years, 6 months ago (2011-04-05 15:15:10 UTC) #20
niemeyer
> done > > will submit with 4285053 and 4343047 once they are ready Have ...
13 years, 6 months ago (2011-04-05 15:48:43 UTC) #21
rsc
> Have you seen my comments? Do they make any sense? sorry, they blurred into ...
13 years, 6 months ago (2011-04-05 15:50:09 UTC) #22
rsc
On Mon, Apr 4, 2011 at 12:23, <n13m3y3r@gmail.com> wrote: > Also, just as a reminder, ...
13 years, 6 months ago (2011-04-05 15:50:56 UTC) #23
niemeyer
> This is still planned. There is even a BUG comment > in the new ...
13 years, 6 months ago (2011-04-05 16:09:36 UTC) #24
rsc
> Ok, thanks. But I was really talking about the actual > inline review of ...
13 years, 6 months ago (2011-04-05 16:10:54 UTC) #25
rsc
http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go File src/pkg/reflect/type.go (right): http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newcode122 src/pkg/reflect/type.go:122: // Elem returns a type's element type. On 2011/04/04 ...
13 years, 6 months ago (2011-04-05 16:29:45 UTC) #26
gustavo_niemeyer.net
Thanks for the response. Understood all points, with some follow ups below: >> clear that ...
13 years, 6 months ago (2011-04-05 16:53:59 UTC) #27
rsc
> Call it an "untyped" zero value. > > Value{}.Kind() == reflect.Untyped (or NoKind, or ...
13 years, 6 months ago (2011-04-05 16:58:32 UTC) #28
niemeyer
On Tue, Apr 5, 2011 at 13:53, Gustavo Niemeyer <gustavo@niemeyer.net> wrote: > Thanks for the ...
13 years, 6 months ago (2011-04-05 16:59:53 UTC) #29
gustavo_niemeyer.net
> I responded to your claim that it was confusing. > Now you say counterintuitive. ...
13 years, 6 months ago (2011-04-05 17:09:41 UTC) #30
rsc
Thanks for bearing with me. The zero Value represents no actual value. I wish I ...
13 years, 6 months ago (2011-04-05 19:46:38 UTC) #31
r
LGTM although i'd like to know why Bits is problematic or unwise for pointers. http://codereview.appspot.com/4281055/diff/9004/src/pkg/reflect/value.go ...
13 years, 6 months ago (2011-04-05 20:33:06 UTC) #32
rsc
> although i'd like to know why Bits is problematic or unwise for > pointers. ...
13 years, 6 months ago (2011-04-05 20:50:59 UTC) #33
r2
LGTM
13 years, 6 months ago (2011-04-05 20:53:46 UTC) #34
gustavo_niemeyer.net
> as their spelling changed. But I think you are > right that this one ...
13 years, 6 months ago (2011-04-05 21:12:43 UTC) #35
rsc
13 years, 6 months ago (2011-04-08 16:26:58 UTC) #36
*** Submitted as http://code.google.com/p/go/source/detail?r=843855f3c026 ***

reflect: new Type and Value definitions

Type is now an interface that implements all the possible type methods.
Instead of a type switch on a reflect.Type t, switch on t.Kind().
If a method is invoked on the wrong kind of type (for example,
calling t.Field(0) when t.Kind() != Struct), the call panics.

There is one method renaming: t.(*ChanType).Dir() is now t.ChanDir().

Value is now a struct value that implements all the possible value methods.
Instead of a type switch on a reflect.Value v, switch on v.Kind().
If a method is invoked on the wrong kind of value (for example,
calling t.Recv() when t.Kind() != Chan), the call panics.

Since Value is now a struct, not an interface, its zero value
cannot be compared to nil.  Instead of v != nil, use v.IsValid().
Instead of other uses of nil as a Value, use Value{}, the zero value.

Many methods have been renamed, most due to signature conflicts:

           OLD                          NEW

    v.(*ArrayValue).Elem             v.Index
    v.(*BoolValue).Get               v.Bool
    v.(*BoolValue).Set               v.SetBool
    v.(*ChanType).Dir                v.ChanDir
    v.(*ChanValue).Get               v.Pointer
    v.(*ComplexValue).Get            v.Complex
    v.(*ComplexValue).Overflow       v.OverflowComplex
    v.(*ComplexValue).Set            v.SetComplex
    v.(*FloatValue).Get              v.Float
    v.(*FloatValue).Overflow         v.OverflowFloat
    v.(*FloatValue).Set              v.SetFloat
    v.(*FuncValue).Get               v.Pointer
    v.(*InterfaceValue).Get          v.InterfaceData
    v.(*IntValue).Get                v.Int
    v.(*IntValue).Overflow           v.OverflowInt
    v.(*IntValue).Set                v.SetInt
    v.(*MapValue).Elem               v.MapIndex
    v.(*MapValue).Get                v.Pointer
    v.(*MapValue).Keys               v.MapKeys
    v.(*MapValue).SetElem            v.SetMapIndex
    v.(*PtrValue).Get                v.Pointer
    v.(*SliceValue).Elem             v.Index
    v.(*SliceValue).Get              v.Pointer
    v.(*StringValue).Get             v.String
    v.(*StringValue).Set             v.SetString
    v.(*UintValue).Get               v.Uint
    v.(*UintValue).Overflow          v.OverflowUint
    v.(*UintValue).Set               v.SetUint
    v.(*UnsafePointerValue).Get      v.Pointer
    v.(*UnsafePointerValue).Set      v.SetPointer

Part of the motivation for this change is to enable a more
efficient implementation of Value, one that does not allocate
memory during most operations.  To reduce the size of the CL,
this CL's implementation is a wrapper around the old API.
Later CLs will make the implementation more efficient without
changing the API.

Other CLs to be submitted at the same time as this one
add support for this change to gofix (4343047) and update
the Go source tree (4353043).

R=gri, iant, niemeyer, r, rog, gustavo, r2
CC=golang-dev
http://codereview.appspot.com/4281055
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b