code review 144020043: reflect: add Type.IsComparable

reflect: add Type.Comparable

Like most of the Type methods, the definition of Comparable
is what the Go spec says it is.

Fixes issue 7911.

[rsc, 2014-09-16 19:41:49]

Hello gri, r (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[r, 2014-09-16 19:44:22]

Does this fix a problem? Otherwise it should wait for 1.5, or what is
a freeze for?

-rob


On Tue, Sep 16, 2014 at 12:41 PM,  <rsc@golang.org> wrote:
> Reviewers: gri, r,
>
> Message:
> Hello gri, r (cc: golang-codereviews@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> reflect: add Type.IsComparable
>
> Like most of the Type methods, the definition of Comparable
> is what the Go spec says it is.
>
> Fixes issue 7911.
>
> Please review this at https://codereview.appspot.com/144020043/
>
> Affected files (+55, -1 lines):
>   M src/reflect/all_test.go
>   M src/reflect/type.go
>
>
> Index: src/reflect/all_test.go
> ===================================================================
> --- a/src/reflect/all_test.go
> +++ b/src/reflect/all_test.go
> @@ -3185,6 +3185,44 @@
>         }
>  }
>
> +type ComparableStruct struct {
> +       X int
> +}
> +
> +type NonComparableStruct struct {
> +       X int
> +       Y map[string]int
> +}
> +
> +var comparableTests = []struct {
> +       typ Type
> +       ok  bool
> +}{
> +       {TypeOf(1), true},
> +       {TypeOf("hello"), true},
> +       {TypeOf(new(byte)), true},
> +       {TypeOf((func())(nil)), false},
> +       {TypeOf([]byte{}), false},
> +       {TypeOf(map[string]int{}), false},
> +       {TypeOf(make(chan int)), true},
> +       {TypeOf(1.5), true},
> +       {TypeOf(false), true},
> +       {TypeOf(1i), true},
> +       {TypeOf(ComparableStruct{}), true},
> +       {TypeOf(NonComparableStruct{}), false},
> +       {TypeOf([10]map[string]int{}), false},
> +       {TypeOf([10]string{}), true},
> +       {TypeOf(new(interface{})).Elem(), true},
> +}
> +
> +func TestIsComparable(t *testing.T) {
> +       for _, tt := range comparableTests {
> +               if ok := tt.typ.IsComparable(); ok != tt.ok {
> +                       t.Errorf("TypeOf(%v).IsComparable() = %v, want %v",
> tt.typ, ok, tt.ok)
> +               }
> +       }
> +}
> +
>  func TestOverflow(t *testing.T) {
>         if ovf := V(float64(0)).OverflowFloat(1e300); ovf {
>                 t.Errorf("%v wrongly overflows float64", 1e300)
> Index: src/reflect/type.go
> ===================================================================
> --- a/src/reflect/type.go
> +++ b/src/reflect/type.go
> @@ -96,6 +96,9 @@
>         // ConvertibleTo returns true if a value of the type is convertible
> to type u.
>         ConvertibleTo(u Type) bool
>
> +       // IsComparable returns true if values of this type are comparable.
> +       IsComparable() bool
> +
>         // Methods applicable only to some types, depending on Kind.
>         // The methods allowed for each kind are:
>         //
> @@ -248,7 +251,7 @@
>         align         uint8             // alignment of variable with this
> type
>         fieldAlign    uint8             // alignment of struct field with
> this type
>         kind          uint8             // enumeration for C
> -       alg           *uintptr          // algorithm table
> (../runtime/runtime.h:/Alg)
> +       alg           *typeAlg          // algorithm table
> (../runtime/runtime.h:/Alg)
>         gc            [2]unsafe.Pointer // garbage collection data
>         string        *string           // string form; unnecessary but
> undeniably useful
>         *uncommonType                   // (relatively) uncommon fields
> @@ -256,6 +259,15 @@
>         zero          unsafe.Pointer    // pointer to zero value
>  }
>
> +type typeAlg struct {
> +       // function for hashing objects of this type
> +       // (ptr to object, size, seed) -> hash
> +       hash func(unsafe.Pointer, uintptr, uintptr) uintptr
> +       // function for comparing objects of this type
> +       // (ptr to object A, ptr to object B, size) -> ==?
> +       equal func(unsafe.Pointer, unsafe.Pointer, uintptr) bool
> +}
> +
>  // Method on non-interface type
>  type method struct {
>         name    *string        // name of method
> @@ -1096,6 +1108,10 @@
>         return convertOp(uu, t) != nil
>  }
>
> +func (t *rtype) IsComparable() bool {
> +       return t.alg != nil && t.alg.equal != nil
> +}
> +
>  // implements returns true if the type V implements the interface type T.
>  func implements(T, V *rtype) bool {
>         if T.Kind() != Interface {
>
>

[gri, 2014-09-16 20:25:00]

LGTM

I'm ok with this change since it's fixing an issue and doesn't seem to pose any
obvious risks (no code is really dependent on it yet).

Leaving for r for final judgement.

[gri, 2014-09-16 20:27:23]

https://codereview.appspot.com/144020043/diff/40001/src/reflect/type.go
File src/reflect/type.go (right):

https://codereview.appspot.com/144020043/diff/40001/src/reflect/type.go#newco...
src/reflect/type.go:100: IsComparable() bool
PS: What is the principle used for naming here? Should it just be Comparable() ?
(it's not IsAssignableTo, after all).

[r, 2014-09-16 20:35:51]

ok, i see the issue now. missed it before

i agree the name doesn't seem idiomatic.

[rsc, 2014-09-16 21:34:08]

Comparable works for me

[rsc, 2014-09-16 21:40:15]

*** Submitted as https://code.google.com/p/go/source/detail?r=aadf24fd5f0b ***

reflect: add Type.Comparable

Like most of the Type methods, the definition of Comparable
is what the Go spec says it is.

Fixes issue 7911.

LGTM=gri
R=gri, r
CC=golang-codereviews
https://codereview.appspot.com/144020043