code review 4281055: reflect: new Type and Value definitions

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, 2011-03-19 06:21:04]

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.

[rsc, 2011-03-19 06:54:31]

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

[niemeyer, 2011-03-19 16:23:10]

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.

[rsc, 2011-03-19 16:47:09]

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.

[iant, 2011-03-20 04:30:39]

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.

[rsc, 2011-03-20 04:40:47]

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

[gri, 2011-03-21 18:56:29]

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)

[rsc, 2011-04-02 01:02:14]

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/

[rsc, 2011-04-02 01:08:03]

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).

[rsc, 2011-04-02 01:09:17]

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

[rsc, 2011-04-02 01:13:56]

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
 }

[r, 2011-04-02 18:45:33]

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?

[klownkeeper_gmail.com, 2011-04-03 04:28:29]

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

[rsc, 2011-04-03 15:18:17]

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

[niemeyer, 2011-04-04 13:52:34]

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.

[niemeyer, 2011-04-04 16:23:46]

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/

[rog, 2011-04-04 17:06:49]

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.

[rsc, 2011-04-04 20:14:32]

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

[r, 2011-04-04 23:26:53]

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'.

[rsc, 2011-04-05 15:15:10]

done

will submit with 4285053 and 4343047 once they are ready

[niemeyer, 2011-04-05 15:48:43]

> done
> 
> will submit with 4285053 and 4343047 once they are ready

Have you seen my comments?  Do they make any sense?

[rsc, 2011-04-05 15:50:09]

> Have you seen my comments?  Do they make any sense?

sorry, they blurred into the weekend mail storm.
looking at them now...

[rsc, 2011-04-05 15:50:56]

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

[niemeyer, 2011-04-05 16:09:36]

> 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.

[rsc, 2011-04-05 16:10:54]

> Ok, thanks.  But I was really talking about the actual
> inline review of the changes, right before this message.

Still reading...

[rsc, 2011-04-05 16:29:45]

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.

[gustavo_niemeyer.net, 2011-04-05 16:53:59]

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

[rsc, 2011-04-05 16:58:32]

> 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

[niemeyer, 2011-04-05 16:59:53]

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

[gustavo_niemeyer.net, 2011-04-05 17:09:41]

> 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

[rsc, 2011-04-05 19:46:38]

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

[r, 2011-04-05 20:33:06]

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...

[rsc, 2011-04-05 20:50:59]

> 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

[r2, 2011-04-05 20:53:46]

LGTM

[gustavo_niemeyer.net, 2011-04-05 21:12:43]

> 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

[rsc, 2011-04-08 16:26:58]

*** 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