|
|
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. |
Descriptionreflect: 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 #
MessagesTotal messages: 36
I think this is a variant of an earlier design, the variance being that all the methods are in one interface instead of "sub" interfaces. Am I remembering correctly? I find the one big interface a little odd, and the Kind switches for the methods clunky. But maybe it's a win for clients.
Sign in to reply to this message.
On Sat, Mar 19, 2011 at 02:21, <r@golang.org> wrote: > I think this is a variant of an earlier design, the variance being that > all the methods are in one interface instead of "sub" interfaces. Am I > remembering correctly? Yes. That design predated the type switch. http://goo.gl/X3aw1 When we added type switches, we dropped the Kind method in favor of non-interface types like *ChanType for each kind. Then we decided the different sized types like *Int8Type, *Int16Type, etc were too fine grained, so we merged them all into *IntType and reintroduced Kind. Now we are in fact back where we started except with one mega-interface instead of many sub-interfaces. I think the type switch was a good experiment but ultimately not the right approach. The type switches are more cluttered and more stuttered than switch on Kind, and in practice much code simply asserted conversions to specific types. With the new code those assertions - which were everywhere - go away. The argument for dropping the subtypes is more compelling in the case of Value (it is the only way to remove the allocations), but I think even for Type the notational clarity is a minor win. > I find the one big interface a little odd, and the Kind switches for the > methods clunky. But maybe it's a win for clients. I just noticed that the client CL didn't upload. Uploaded now: http://codereview.appspot.com/4301043 It's almost always cleaner. The exception is looking for int and uint types. (And that's a mechanical CL; there is room for improvement in some of the code, which used to do switch on Kind inside the type switch.) My favorite rewrite: diff -r f692a5e90f6f src/pkg/json/encode.go --- a/src/pkg/json/encode.go Fri Mar 18 20:55:21 2011 -0400 +++ b/src/pkg/json/encode.go Sat Mar 19 02:52:55 2011 -0400 @@ -247,7 +247,7 @@ e.WriteByte('}') case *reflect.MapValue: - if _, ok := v.Type().(*reflect.MapType).Key().(*reflect.StringType); !ok { + if v.Type().Key().Kind() != reflect.String { e.error(&UnsupportedTypeError{v.Type()}) } if v.IsNil() { Russ
Sign in to reply to this message.
While the amount of clutter removed feels very good, part of me is wondering about the underlying message being passed with this new design. This is unlike anything else we have at the moment, and feels like compromising in some important areas. - if _, ok := v.Type().(*reflect.MapType).Key(.(*reflect.StringType); !ok { + if v.Type().Key().Kind() != reflect.String { Even if cluttered and assertive, the former line is idiomatic Go which offers independent types and interfaces for values which are structured and behave differently. The new version uses a conflated type and interface and puts the burn of knowing what the internal value offers on the brain of the programmer. As an example, every type now has Len(), Size(), and Bits(), and each of these work in a different selection of "internal actual types". Panics at runtime only. Also, one can't use the common idiom of inquiring the type's interface anymore. This is from a real package, for instance: type hasIsNil interface { IsNil() bool } I certainly agree that reflect is an interesting problem, but this is the kind of tricky case that should lead to improvements in the language, if necessary. It feels like this is instead bailing out from the common idioms the language is structured around and that we advise people to use because they are not suitable for non-trivial problems.
Sign in to reply to this message.
You're right but I think reflect is always a bit different. It exists to do at run time what usually happens at compile time.
Sign in to reply to this message.
With this approach, why retain the distinction between the runtime type structures and the reflect type structures, where the reflect structures are copies of the runtime ones? When reflect.Type is an interface it seems we may as well just point back to runtime type structures in all cases.
Sign in to reply to this message.
On Sun, Mar 20, 2011 at 00:30, <iant@golang.org> wrote: > With this approach, why retain the distinction between the runtime type > structures and the reflect type structures, where the reflect structures > are copies of the runtime ones? When reflect.Type is an interface it > seems we may as well just point back to runtime type structures in all > cases. Yes, I'd like to do that. It's a little tricky, though, because you can't make the interfaces contain public runtime types and allow copying of structs containing unexported fields. If you do that, clients can overwrite one *ChanType with another by copying. One possibility is to eliminate the Go definitions in package runtime and say that runtime types can only be interpreted by reflect. I'm not sure. It's definitely something to think about. Russ
Sign in to reply to this message.
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#newco... src/pkg/reflect/type.go:74: // a value of the given type; it is analogous to unsafe.Sizeof. ...; the result is the same as returned by unsafe.Sizeof for a value of the type. ??? http://codereview.appspot.com/4281055/diff/1001/src/pkg/reflect/type.go#newco... src/pkg/reflect/type.go:120: DotDotDot() bool s/DotDotDot/Variadic/ ? (an array type for [...]int{1, 2, 3} also has a ... - I understand that the compiler will simply fill in the right length in this case but DotDotDot seems a bit odd to me)
Sign in to reply to this message.
Hello golang-dev, gri, iant, niemeyer, r (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
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).
Sign in to reply to this message.
My plan is to submit these changes (and the related CLs, which I'll send out for review Monday morning) *after* the next weekly. Russ
Sign in to reply to this message.
For what it's worth, these are the diffs necessary to the tree to convert to the new API, after running gofix. --- src/pkg/fmt/print.go 2011-04-01 15:34:30.000000000 -0400 +++ src/pkg/fmt/print.go 2011-04-01 15:36:36.000000000 -0400 @@ -278,11 +278,6 @@ return } -// Reflection values like reflect.FuncValue implement this method. We use it for %p. -type uintptrGetter interface { - Get() uintptr -} - func (p *pp) unknownType(v interface{}) { if v == nil { p.buf.Write(nilAngleBytes) @@ -523,7 +518,7 @@ var u uintptr switch value.Kind() { case reflect.Chan, reflect.Func, reflect.Map, reflect.Ptr, reflect.Slice, reflect.UnsafePointer: - u = value.(uintptrGetter).Get() + u = value.Pointer() default: p.badVerb(verb, field) return --- src/pkg/json/encode.go 2011-04-01 15:39:55.000000000 -0400 +++ src/pkg/json/encode.go 2011-04-01 15:39:58.000000000 -0400 @@ -296,7 +296,7 @@ } e.WriteByte(']') - case interfaceOrPtrValue: + case reflect.Interface, reflect.Ptr: if v.IsNil() { e.WriteString("null") return --- src/pkg/json/decode.go 2011-04-01 15:38:51.000000000 -0400 +++ src/pkg/json/decode1.go 2011-04-01 15:40:39.000000000 -0400 @@ -560,7 +560,7 @@ default: d.saveError(&UnmarshalTypeError{"null", v.Type()}) case reflect.Interface, reflect.Ptr, reflect.Map: - v.Set(nil) + v.Set(reflect.Value{}) } case 't', 'f': // true, false --- src/pkg/go/ast/print.go 2011-04-01 15:48:48.000000000 -0400 +++ src/pkg/go/ast/print.go 2011-04-01 15:55:32.000000000 -0400 @@ -21,11 +21,12 @@ // NotNilFilter returns true for field values that are not nil; // it returns false otherwise. -func NotNilFilter(_ string, value reflect.Value) bool { - v, ok := value.(interface { - IsNil() bool - }) - return !ok || !v.IsNil() +func NotNilFilter(_ string, v reflect.Value) bool { + switch v.Kind() { + case reflect.Chan, reflect.Func, reflect.Interface, reflect.Map, reflect.Ptr, reflect.Slice: + return !v.IsNil() + } + return false }
Sign in to reply to this message.
be interesting to know if this makes a measurable performance difference on, say, the JSON package. perhaps worth adding a benchmark and malloc counter test and reporting before-and-after numbers. i remember the last time we sped up reflection, it slowed stuff down 40% 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#newc... src/pkg/reflect/type.go:23: We want to be able to call methods on Type and to compare types for equality. i think this comment can go. at least it needs rephrasing. http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... src/pkg/reflect/type.go:34: // Type is the representation of a Go type. might be good to have helper methods or functions for classes of type: IsBasic IsInteger IsFloating IsComplex http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... src/pkg/reflect/type.go:40: // inappropriate to the kind of type causes a run time panic. s/run time/run-time/ (adjective) http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... src/pkg/reflect/type.go:45: // when allocated in memory. what units? http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... src/pkg/reflect/type.go:49: // when used as a field in a struct. what units? http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... src/pkg/reflect/type.go:52: // Method returns the i'th method in the type's method set. what happens if i is out of range? obviously, it panics, but reflection code needs good documentation, so say so here. http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... src/pkg/reflect/type.go:64: // Name returns the type's name within its package. really? i thought it returned pkg.NameOfType http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... src/pkg/reflect/type.go:120: DotDotDot() bool wrong name for a boolean method. HasDotDotDot is longer but clearer. http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... src/pkg/reflect/type.go:126: // Field returns a struct type's i'th field. what if i is out of range? http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... src/pkg/reflect/type.go:146: // It panics if the type's Kind is not Func. here and for Out: what if i is out of range? 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#new... src/pkg/reflect/value.go:58: Internal valueInterface why is Internal exported? http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... src/pkg/reflect/value.go:100: func (v Value) kindPanic(want Kind) valueInterface { this name reads poorly when used. i'd called it panicIfNot(want) http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... src/pkg/reflect/value.go:111: func (v Value) kindsPanic(wants []Kind) valueInterface { ditto http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... src/pkg/reflect/value.go:341: // OverflowComplex returns true if the complex128 x cannot be represented by v's type. not clear what this means, exactly. i mean, we have Inf. same question for floats, i'm sure. http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... src/pkg/reflect/value.go:371: // UnsafePointer returns v's value as a uintptr. it's called Pointer http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... src/pkg/reflect/value.go:432: // SetMapElem sets the value associated with key in the map v to val. it's called SetMapIndex http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... src/pkg/reflect/value.go:457: // BUG(rsc): Value.Slice should allow slicing arrays. can you just fix it?
Sign in to reply to this message.
So was taruti right or wrong for saying DotDotDot instead of HasDotDotDot and how much affect does it have? Also why does there seem to be so many run-time errors in golang? Last how do you know when to synthesize your code into an If/return statement? On Sat, Apr 2, 2011 at 2:45 PM, <r@golang.org> wrote: > be interesting to know if this makes a measurable performance difference > on, say, the JSON package. > perhaps worth adding a benchmark and malloc counter test and reporting > before-and-after numbers. > i remember the last time we sped up reflection, it slowed stuff down 40% > > > > 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#newc... > src/pkg/reflect/type.go:23: We want to be able to call methods on Type > and to compare types for equality. > i think this comment can go. at least it needs rephrasing. > > > http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... > src/pkg/reflect/type.go:34: // Type is the representation of a Go type. > might be good to have helper methods or functions for classes of type: > IsBasic > IsInteger > IsFloating > IsComplex > > > http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... > src/pkg/reflect/type.go:40: // inappropriate to the kind of type causes > a run time panic. > s/run time/run-time/ (adjective) > > > http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... > src/pkg/reflect/type.go:45: // when allocated in memory. > what units? > > > http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... > src/pkg/reflect/type.go:49: // when used as a field in a struct. > what units? > > > http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... > src/pkg/reflect/type.go:52: // Method returns the i'th method in the > type's method set. > what happens if i is out of range? > obviously, it panics, but reflection code needs good documentation, so > say so here. > > > http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... > src/pkg/reflect/type.go:64: // Name returns the type's name within its > package. > really? i thought it returned pkg.NameOfType > > > http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... > src/pkg/reflect/type.go:120: DotDotDot() bool > wrong name for a boolean method. HasDotDotDot is longer but clearer. > > > http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... > src/pkg/reflect/type.go:126: // Field returns a struct type's i'th > field. > what if i is out of range? > > > http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... > src/pkg/reflect/type.go:146: // It panics if the type's Kind is not > Func. > here and for Out: what if i is out of range? > > 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#new... > src/pkg/reflect/value.go:58: Internal valueInterface > why is Internal exported? > > > http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... > src/pkg/reflect/value.go:100: func (v Value) kindPanic(want Kind) > valueInterface { > this name reads poorly when used. i'd called it panicIfNot(want) > > > http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... > src/pkg/reflect/value.go:111: func (v Value) kindsPanic(wants []Kind) > valueInterface { > ditto > > > http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... > src/pkg/reflect/value.go:341: // OverflowComplex returns true if the > complex128 x cannot be represented by v's type. > not clear what this means, exactly. i mean, we have Inf. > same question for floats, i'm sure. > > > http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... > src/pkg/reflect/value.go:371: // UnsafePointer returns v's value as a > uintptr. > it's called Pointer > > > http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... > src/pkg/reflect/value.go:432: // SetMapElem sets the value associated > with key in the map v to val. > it's called SetMapIndex > > > http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... > src/pkg/reflect/value.go:457: // BUG(rsc): Value.Slice should allow > slicing arrays. > can you just fix it? > > http://codereview.appspot.com/4281055/ > -- Cordially, Keith Clark Chairman and CEO of Jana Matthew 7:7
Sign in to reply to this message.
On Sun, Apr 3, 2011 at 00:28, Keith Clark <klownkeeper@gmail.com> wrote: > So was taruti right or wrong for saying DotDotDot instead of HasDotDotDot > and how much affect does it have? Also why does there seem to be so many > run-time errors in golang? Last how do you know when to synthesize your code > into an If/return statement? I think these questions belong more on golang-nuts. Please take them there. Russ
Sign in to reply to this message.
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#... src/pkg/reflect/all_test.go:217: switch v.Kind() { Nice, that's much cleaner. 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#newc... src/pkg/reflect/type.go:122: // Elem returns a type's element type. s/element type/element Type/ ? http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... src/pkg/reflect/type.go:151: Key() Type Should this be called MapKey, or MapKeys be renamed to Keys, for symmetry? http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... src/pkg/reflect/type.go:524: panic("reflect: Field of non-struct type") It would be helpful to have a TypeError analogous to ValueError, which presents what the actual type is in its String method. It will certainly save debugging time to know what the unpredicted type was. http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... src/pkg/reflect/type.go:690: return StructField{} Should we take the chance to make this panic rather than returning a zero StructField? It's not clear what would be the point of asking the field of a non-struct, and the behavior is not even documented. 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#new... src/pkg/reflect/value.go:54: // its String method returns "<invalid Value>", and all other methods panic. The usage of the zero value Value{} as meaning something invalid feels a bit counterintuitive, and given some of the tests which use Value{} it's not even clear that *invalid* is indeed what this means. Then, it also feels a bit strange because it seems to confuse the implementation of the reflection package with the semantic meaning of what is being done (a zero Value is not a zero value, if you see what I mean). What about having an explicit reflect.NilValue variable or similar, and then hiding the Internal attribute so that people can't construct these values externally? The implementation might actually stay exactly the same right now, but it would be hidden behind an interface which describes better the intended semantics. http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... src/pkg/reflect/value.go:58: Internal valueInterface On 2011/04/02 18:45:33, r wrote: > why is Internal exported? Likely to allow the use of Value{}, but indeed it feels like it should be hidden and something like described above done instead. http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... src/pkg/reflect/value.go:156: func (v Value) CanSet() bool { Can we have a CanBeNil() or similar in the same spirit of CanSet() and CanAddr(), which enables calling IsNil() in a safe way? The usual trick of using an interface won't work anymore, and it seems to be used both within the standard library and in external packages. http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... src/pkg/reflect/value.go:279: // It panics if v's Kind is not Chan, Func, Interface, Map, Ptr, or Slice. If CanBeNil is introduced, this may be documented in terms of it. http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... src/pkg/reflect/value.go:341: // OverflowComplex returns true if the complex128 x cannot be represented by v's type. s/type/Kind/ (v's type is Value). Same for other Overflow functions below. http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... src/pkg/reflect/value.go:396: // Set assigns x to the value v; x must have the same type as v. s/same type/same Kind/ http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... src/pkg/reflect/value.go:434: // If val is the zero Value, SetMapElem deletes the key from the map. Some of the semantic confusion described can be seen here. The zero Value is not the same as the zero value. The key is deleted with a zero Value, but inserted with a Value which represents the zero value for the given type. http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... src/pkg/reflect/value.go:496: // Type returns v's type. s/v's type/v's Type value/ ? http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... src/pkg/reflect/value.go:1858: // MakeZero returns a zero Value for the specified Type. More confusion. Now the zero Value is actually the Value containing the zero value for the type, not the zero Value as described elsewhere.
Sign in to reply to this message.
Also, just as a reminder, the following issue was targeted at the next redesign, so it may be worth at least pondering about its integration path: http://code.google.com/p/go/issues/detail?id=1432 It was prompted by this CL which was postponed: http://codereview.appspot.com/3608041/
Sign in to reply to this message.
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#new... src/pkg/reflect/value.go:58: Internal valueInterface > why is Internal exported? we don't allow passing external structs with unexported fields by value (yet?). you can't hide it and still avoid the allocations. http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... src/pkg/reflect/value.go:156: func (v Value) CanSet() bool { an alternative might be to have IsNil return false for types that can't be nil.
Sign in to reply to this message.
On Sat, Apr 2, 2011 at 14:45, <r@golang.org> wrote: > be interesting to know if this makes a measurable performance difference > on, say, the JSON package. > perhaps worth adding a benchmark and malloc counter test and reporting > before-and-after numbers. > i remember the last time we sped up reflection, it slowed stuff down 40% there's no perfornace win in this CL. it is probably a little slower. i was trying to keep the changes in this CL as small as possible (they are still quite large) so i wrapped the old API instead of implementing the new one directly. i can make more changes in this CL if you'd prefer. you'd probably want to view the intra-version diffs in that case. > http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... > src/pkg/reflect/type.go:23: We want to be able to call methods on Type > and to compare types for equality. > i think this comment can go. at least it needs rephrasing. gone. > http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... > src/pkg/reflect/type.go:34: // Type is the representation of a Go type. > might be good to have helper methods or functions for classes of type: > IsBasic > IsInteger > IsFloating > IsComplex maybe add in a new CL? > http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... > src/pkg/reflect/type.go:40: // inappropriate to the kind of type causes > a run time panic. > s/run time/run-time/ (adjective) > http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... > src/pkg/reflect/type.go:45: // when allocated in memory. > what units? bytes, added. > http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... > src/pkg/reflect/type.go:49: // when used as a field in a struct. > what units? bytes, added. > http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... > src/pkg/reflect/type.go:52: // Method returns the i'th method in the > type's method set. > what happens if i is out of range? > obviously, it panics, but reflection code needs good documentation, so > say so here. done > http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... > src/pkg/reflect/type.go:64: // Name returns the type's name within its > package. > really? i thought it returned pkg.NameOfType that's what String returns. these comments are all from the original API. > http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... > src/pkg/reflect/type.go:120: DotDotDot() bool > wrong name for a boolean method. HasDotDotDot is longer but clearer. this was an original name too. robert asked for IsVariadic. changed to that. > http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... > src/pkg/reflect/type.go:126: // Field returns a struct type's i'th > field. > what if i is out of range? panics, documented. > http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... > src/pkg/reflect/type.go:146: // It panics if the type's Kind is not > Func. > here and for Out: what if i is out of range? panics, documented. > http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... > src/pkg/reflect/value.go:58: Internal valueInterface > why is Internal exported? because if it is not exported, Value is not copyable. > http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... > src/pkg/reflect/value.go:100: func (v Value) kindPanic(want Kind) > valueInterface { > this name reads poorly when used. i'd called it panicIfNot(want) will go away. renamed. > http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... > src/pkg/reflect/value.go:111: func (v Value) kindsPanic(wants []Kind) > valueInterface { > ditto renamed. > http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... > src/pkg/reflect/value.go:341: // OverflowComplex returns true if the > complex128 x cannot be represented by v's type. > not clear what this means, exactly. i mean, we have Inf. > same question for floats, i'm sure. it means that you can put x into v and pull it out again and get the same value back +/- 1/2 ULP. if v is a float32, v.Overflow(Inf) is false, because you can represent Inf in a float32. on the other hand, v.Overflow(1e300) is true, because there is no float32 anywhere near 1e300. http://golang.org/doc/play/#package%20main%0A%0Avar%20f%20float32%20%3D%201e3... > http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... > src/pkg/reflect/value.go:371: // UnsafePointer returns v's value as a > uintptr. > it's called Pointer so it is. done. > http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... > src/pkg/reflect/value.go:432: // SetMapElem sets the value associated > with key in the map v to val. > it's called SetMapIndex so it is. done. > http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... > src/pkg/reflect/value.go:457: // BUG(rsc): Value.Slice should allow > slicing arrays. > can you just fix it? no, not yet. i need something equivalent to PtrTo to make the slice type given the element type. russ
Sign in to reply to this message.
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#newc... src/pkg/reflect/type.go:90: // sized or unsized Int, Uint, Float, or Complex knids. knids is a good word. pointers are ok too, no? http://codereview.appspot.com/4281055/diff/18002/src/pkg/reflect/type.go#newc... src/pkg/reflect/type.go:99: // underlying static type []T. s/underlying static/implicit actual/ ? avoid 'static'.
Sign in to reply to this message.
done will submit with 4285053 and 4343047 once they are ready
Sign in to reply to this message.
> done > > will submit with 4285053 and 4343047 once they are ready Have you seen my comments? Do they make any sense?
Sign in to reply to this message.
> Have you seen my comments? Do they make any sense? sorry, they blurred into the weekend mail storm. looking at them now...
Sign in to reply to this message.
On Mon, Apr 4, 2011 at 12:23, <n13m3y3r@gmail.com> wrote: > Also, just as a reminder, the following issue was targeted at the next > redesign, so it may be worth at least pondering about its integration > path: > > http://code.google.com/p/go/issues/detail?id=1432 > > It was prompted by this CL which was postponed: > > http://codereview.appspot.com/3608041/ This is still planned. There is even a BUG comment in the new CL. Russ
Sign in to reply to this message.
> This is still planned. There is even a BUG comment > in the new CL. Ok, thanks. But I was really talking about the actual inline review of the changes, right before this message.
Sign in to reply to this message.
> Ok, thanks. But I was really talking about the actual > inline review of the changes, right before this message. Still reading...
Sign in to reply to this message.
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#newc... src/pkg/reflect/type.go:122: // Elem returns a type's element type. On 2011/04/04 13:52:35, niemeyer wrote: > s/element type/element Type/ ? I intentionally left type lower case in most places, since it was pretty awkward to draw a line. The return type speaks for itself. http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... src/pkg/reflect/type.go:151: Key() Type On 2011/04/04 13:52:35, niemeyer wrote: > Should this be called MapKey, or MapKeys be renamed to Keys, for symmetry? I don't think so. SliceType still uses Elem() to get the element type but Index(i) to index into it. They're just different. http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... src/pkg/reflect/type.go:524: panic("reflect: Field of non-struct type") On 2011/04/04 13:52:35, niemeyer wrote: > It would be helpful to have a TypeError analogous to ValueError, which presents > what the actual type is in its String method. It will certainly save debugging > time to know what the unpredicted type was. Sure, I will add that in a separate CL. http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/type.go#newc... src/pkg/reflect/type.go:690: return StructField{} On 2011/04/04 13:52:35, niemeyer wrote: > Should we take the chance to make this panic rather than returning a zero > StructField? It's not clear what would be the point of asking the field of a > non-struct, and the behavior is not even documented. Done. 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#new... src/pkg/reflect/value.go:54: // its String method returns "<invalid Value>", and all other methods panic. On 2011/04/04 13:52:35, niemeyer wrote: > The usage of the zero value Value{} as meaning something invalid feels a bit > counterintuitive, and given some of the tests which use Value{} it's not even > clear that *invalid* is indeed what this means. Suggest a better name? > Then, it also feels a bit strange because it seems to confuse the implementation > of the reflection package with the semantic meaning of what is being done (a > zero Value is not a zero value, if you see what I mean). I don't see what you mean. IsValid means the Value refers to an actual Go value (as opposed to no Go value). The zero Value is a meaningless Value, quite literally. > What about having an explicit reflect.NilValue variable or similar, and then > hiding the Internal attribute so that people can't construct these values > externally? > > The implementation might actually stay exactly the same right now, but it would > be hidden behind an interface which describes better the intended semantics. The whole point of changing the API is to make Value a struct so that it can be created and returned without any allocation. Hiding anything behind an interface implies an allocation. That's exactly what we're trying to get rid of. http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... src/pkg/reflect/value.go:156: func (v Value) CanSet() bool { On 2011/04/04 13:52:35, niemeyer wrote: > Can we have a CanBeNil() or similar in the same spirit of CanSet() and > CanAddr(), which enables calling IsNil() in a safe way? The usual trick of > using an interface won't work anymore, and it seems to be used both within the > standard library and in external packages. I think by "usual trick" you mean testing for the method. That idiom depends on convention for its power, and we are changing the convention. Existing code will have to change in this respect. However, gofix is very likely to point it out and if not the code will not compile anyway. I don't want to add CanBeNil, CanDoThis, CanDoThat. Code can already figure out whether IsNil can be called by looking at Kind. The fact that the code then has to list the kinds it really cares about nils for strikes me as a good thing. Most code that is handling any kind of nil is wrong anyway. http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... src/pkg/reflect/value.go:341: // OverflowComplex returns true if the complex128 x cannot be represented by v's type. On 2011/04/04 13:52:35, niemeyer wrote: > s/type/Kind/ (v's type is Value). > > Same for other Overflow functions below. You're being pedantic. v refers to the underlying value at the same time that it refers to the variable of type Value. The choice of meaning here is clear, because the other meaning is obviously wrong. http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... src/pkg/reflect/value.go:396: // Set assigns x to the value v; x must have the same type as v. On 2011/04/04 13:52:35, niemeyer wrote: > s/same type/same Kind/ No. It must have the same type. You can't assign one struct to a different struct even though they will have the same Kind. http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... src/pkg/reflect/value.go:434: // If val is the zero Value, SetMapElem deletes the key from the map. On 2011/04/04 13:52:35, niemeyer wrote: > Some of the semantic confusion described can be seen here. The zero Value is > not the same as the zero value. The key is deleted with a zero Value, but > inserted with a Value which represents the zero value for the given type. I really don't think this is confusing. The text at the top introduced "the zero Value" as a concept with a very clear definition, and the rest of this file uses that exact phrase when referring to it. Yes, "the zero Value" is different from reflect.NewValue(0). That's just the way it is. http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... src/pkg/reflect/value.go:496: // Type returns v's type. On 2011/04/04 13:52:35, niemeyer wrote: > s/v's type/v's Type value/ ? No. Same as above. http://codereview.appspot.com/4281055/diff/12001/src/pkg/reflect/value.go#new... src/pkg/reflect/value.go:1858: // MakeZero returns a zero Value for the specified Type. On 2011/04/04 13:52:35, niemeyer wrote: > More confusion. Now the zero Value is actually the Value containing the zero > value for the type, not the zero Value as described elsewhere. Thanks, this one is wrong. Changed. // MakeZero returns a Value representing the zero value for the specified type.
Sign in to reply to this message.
Thanks for the response. Understood all points, with some follow ups below: >> clear that *invalid* is indeed what this means. > > Suggest a better name? Call it an "untyped" zero value. Value{}.Kind() == reflect.Untyped (or NoKind, or similar) Get rid of IsValid(). All values are valid, since you're using them in assignments in perfectly valid situations. >> zero Value is not a zero value, if you see what I mean). > > I don't see what you mean. Right now a "zero Value" is an "invalid value" rather than a "zero value". >> be hidden behind an interface which describes better the intended > semantics. > > The whole point of changing the API is to make Value a struct > so that it can be created and returned without any allocation. I meant "interface" as in API, not as in interface{}. > I really don't think this is confusing. The text at the top introduced > "the zero Value" as a concept with a very clear definition, and the > rest of this file uses that exact phrase when referring to it. The fact you're documenting something counterintuitive doesn't turn it into something intuitive. -- Gustavo Niemeyer http://niemeyer.net http://niemeyer.net/blog http://niemeyer.net/twitter
Sign in to reply to this message.
> Call it an "untyped" zero value. > > Value{}.Kind() == reflect.Untyped (or NoKind, or similar) That has the wrong connotations to me. >> I really don't think this is confusing. The text at the top introduced >> "the zero Value" as a concept with a very clear definition, and the >> rest of this file uses that exact phrase when referring to it. > > The fact you're documenting something counterintuitive doesn't turn it > into something intuitive. I responded to your claim that it was confusing. Now you say counterintuitive. Those are not the same. I can't aspire to intuitive, since everyone has different backgrounds and preconceptions that color what they expect. I can only aspire to explaining it well. Russ
Sign in to reply to this message.
On Tue, Apr 5, 2011 at 13:53, Gustavo Niemeyer <gustavo@niemeyer.net> wrote: > Thanks for the response. Understood all points, with some follow ups below: > >>> clear that *invalid* is indeed what this means. >> >> Suggest a better name? > > Call it an "untyped" zero value. > > Value{}.Kind() == reflect.Untyped (or NoKind, or similar) > > Get rid of IsValid(). All values are valid, since you're using them > in assignments in perfectly valid situations. Just to be clear, I was referring to this kind of use: + vp := NewValue(ip) + vp.Set(Value{}) (...) - vip.(*PtrValue).Elem().(*PtrValue).Set(nil) + vip.Elem().Set(Value{}) (...) + newmap.SetMapIndex(NewValue("a"), Value{}) Value{} is really being used as a "zero value" in these cases, not as something invalid. -- Gustavo Niemeyer http://niemeyer.net http://niemeyer.net/blog http://niemeyer.net/twitter
Sign in to reply to this message.
> I responded to your claim that it was confusing. > Now you say counterintuitive. Those are not the same. Sorry, I'm just trying to help clarifying the conflict between a "zero value" and a "zero Value", which feels like an unnecessary minor burden in your overall plan. You can very easily avoid using "zero Value" entirely. > backgrounds and preconceptions that color what they expect. I'm not bikeshedding. I'm just suggesting a clarification in your API for an obvious conflict, but I'll stop arguing now since the point has been made. -- Gustavo Niemeyer http://niemeyer.net http://niemeyer.net/blog http://niemeyer.net/twitter
Sign in to reply to this message.
Thanks for bearing with me. The zero Value represents no actual value. I wish I could call it the missing Value or the empty Value but neither is accurate: it is, in Go terminology, the zero Value. So we have to do what we can with what we have. You are right that there is a conflation of the zero Value and the Value representing the zero of a given type on the part of the Set method for Ptr and Map kinds. This is a dreg from the old API's conflation of the nil Value and the Value representing the nil of a given type. It's unfortunate. I kept it because I was trying to preserve the semantics of existing reflect programs even as their spelling changed. But I think you are right that this one is a mistake worth fixing, so I did. Part of the reason I didn't fix it was that vp.Set(nil) was so much shorter than vp.Set(reflect.MakeZero(vp.Type())). To shorten that I changed MakeZero to Zero. vp.Set(reflect.Zero(vp.Type())). I also expanded the comment on Zero nee MakeZero: // Zero returns a Value representing a zero value for the specified type. // The result is different from the zero Value, which represents no value at all. // For example, Zero(Typeof(42)) returns a Value with Kind Int and value 0. Russ
Sign in to reply to this message.
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 File src/pkg/reflect/value.go (right): http://codereview.appspot.com/4281055/diff/9004/src/pkg/reflect/value.go#newc... src/pkg/reflect/value.go:1849: // The result is different from the zero Value, which represents no value at all. the capitalization makes this hard to read. be more explicit. The result is different from the zero value of the Value struct, which...
Sign in to reply to this message.
> although i'd like to know why Bits is problematic or unwise for > pointers. not necessarily unwise but Size is available to get the byte size. Bits is really for "you know you have an int, but how big is it". Bits used to work on everything but only if the answer didn't overflow. i cut it down to just the kinds of types where you see bit counts in the names (plus the unsized variants int, uint). russ
Sign in to reply to this message.
> as their spelling changed. But I think you are > right that this one is a mistake worth fixing, so I did. Looks great, thank you very much, and sorry for not being clearer at first. -- Gustavo Niemeyer http://niemeyer.net http://niemeyer.net/blog http://niemeyer.net/twitter
Sign in to reply to this message.
*** 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.
|