code review 26570046: reflect: Add tests for Call with functions taking and r...

reflect: Add tests for Call with functions taking and returning structs.

gccgo has problems using reflect.Call with functions that take and
return structs with no members.  Prior to fixing that problem there, I
thought it sensible to add some tests of this situation.

Update issue 6761

First contribution to Go, apologies in advance if I'm doing it wrong.

[mwhudson, 2013-11-15 00:31:45]

Hello golang-dev@googlegroups.com,

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

[dave_cheney.net, 2013-11-15 00:46:47]

Please add

Fixes issue 6761.

to the top of the description.

On Fri, Nov 15, 2013 at 11:31 AM,  <michael.hudson@linaro.org> wrote:
> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> reflect: Add tests for Call with functions taking and returning structs.
>
> gccgo has problems using reflect.Call with functions that take and
> return structs with no members.  Prior to fixing that problem there, I
> thought it sensible to add some tests of this situation.
>
> First contribution to Go, apologies in advance if I'm doing it wrong.
>
> Please review this at https://codereview.appspot.com/26570046/
>
> Affected files (+40, -0 lines):
>   M src/pkg/reflect/all_test.go
>
>
> Index: src/pkg/reflect/all_test.go
> ===================================================================
> --- a/src/pkg/reflect/all_test.go
> +++ b/src/pkg/reflect/all_test.go
> @@ -1434,6 +1434,46 @@
>         }
>  }
>
> +type emptyStruct struct{}
> +
> +type nonEmptyStruct struct {
> +       member int
> +}
> +
> +func returnEmpty() emptyStruct {
> +       return emptyStruct{}
> +}
> +
> +func takesEmpty(e emptyStruct) {
> +}
> +
> +func returnNonEmpty(i int) nonEmptyStruct {
> +       return nonEmptyStruct{member: i}
> +}
> +
> +func takesNonEmpty(n nonEmptyStruct) int {
> +       return n.member
> +}
> +
> +func TestCallWithStruct(t *testing.T) {
> +       r := ValueOf(returnEmpty).Call([]Value{})
> +       if len(r) != 1 || r[0].Type() != TypeOf(emptyStruct{}) {
> +               t.Errorf("returning empty struct returned %s instead", r)
> +       }
> +       r = ValueOf(takesEmpty).Call([]Value{ValueOf(emptyStruct{})})
> +       if len(r) != 0 {
> +               t.Errorf("takesEmpty returned values: %s", r)
> +       }
> +       r = ValueOf(returnNonEmpty).Call([]Value{ValueOf(42)})
> +       if len(r) != 1 || r[0].Type() != TypeOf(nonEmptyStruct{}) ||
> r[0].Field(0).Int() != 42 {
> +               t.Errorf("returnNonEmpty returned %s", r)
> +       }
> +       r =
> ValueOf(takesNonEmpty).Call([]Value{ValueOf(nonEmptyStruct{member: 42})})
> +       if len(r) != 1 || r[0].Type() != TypeOf(1) || r[0].Int() != 42 {
> +               t.Errorf("takesNonEmpty returned %s", r)
> +       }
> +}
> +
>  func TestMakeFunc(t *testing.T) {
>         f := dummy
>         fv := MakeFunc(TypeOf(f), func(in []Value) []Value { return in })
>
>
> --
>
> ---You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.

[minux1, 2013-11-15 00:57:27]

On Nov 14, 2013 7:46 PM, "Dave Cheney" <dave@cheney.net> wrote:
> Please add
>
> Fixes issue 6761.
this is not a fix, so perhaps
Update issue 6761
is more suitable.

[iant, 2013-11-15 01:10:24]

Some suggestions.

Have you signed the contributor agreement?

https://codereview.appspot.com/26570046/diff/40001/src/pkg/reflect/all_test.go
File src/pkg/reflect/all_test.go (right):

https://codereview.appspot.com/26570046/diff/40001/src/pkg/reflect/all_test.g...
src/pkg/reflect/all_test.go:1459: r := ValueOf(returnEmpty).Call([]Value{})
s/[]Value[]/nil/

https://codereview.appspot.com/26570046/diff/40001/src/pkg/reflect/all_test.g...
src/pkg/reflect/all_test.go:1461: t.Errorf("returning empty struct returned %s
instead", r)
Here r is not a string, so %v would be better and in this case %#v would be
better still.

https://codereview.appspot.com/26570046/diff/40001/src/pkg/reflect/all_test.g...
src/pkg/reflect/all_test.go:1465: t.Errorf("takesEmpty returned values: %s", r)
s/%s/%#v/

https://codereview.appspot.com/26570046/diff/40001/src/pkg/reflect/all_test.g...
src/pkg/reflect/all_test.go:1469: t.Errorf("returnNonEmpty returned %s", r)
s/%s/%#v/

https://codereview.appspot.com/26570046/diff/40001/src/pkg/reflect/all_test.g...
src/pkg/reflect/all_test.go:1473: t.Errorf("takesNonEmpty returned %s", r)
s/%s/%#v/

[mwhudson, 2013-11-15 01:12:25]

minux <minux.ma@gmail.com> writes:

> On Nov 14, 2013 7:46 PM, "Dave Cheney" <dave@cheney.net> wrote:
>> Please add
>>
>> Fixes issue 6761.
> this is not a fix, so perhaps
> Update issue 6761
> is more suitable.

Done.

[mwhudson, 2013-11-15 01:34:00]

On 2013/11/15 01:10:24, iant wrote:
> Some suggestions.
> 
> Have you signed the contributor agreement?

No.  For my sins I need to get two corporate CLAs signed for this work (one each
from Canonical and Linaro).  I've started this process.

Your suggestions seem reasonable, I've made the changes.

[mwhudson, 2013-11-17 23:07:50]

iant@golang.org writes:

> Have you signed the contributor agreement?

My organization, Linaro, has now (apparently an agreement from Canonical
is not required for this work).

Cheers,
mwh

[mwhudson, 2013-12-17 03:12:54]

Ping?

[khr, 2013-12-17 22:14:39]

On 2013/12/17 03:12:54, mwhudson wrote:
> Ping?

LGTM

[dave_cheney.net, 2013-12-17 22:18:06]

Can someone please do a A+C for Michael. His details are already on file for
gccgo contributions. 

> On 18 Dec 2013, at 9:14, khr@golang.org wrote:
> 
>> On 2013/12/17 03:12:54, mwhudson wrote:
>> Ping?
> 
> LGTM
> 
> https://codereview.appspot.com/26570046/

[bradfitz, 2013-12-17 22:20:45]

I don't see him in our CLA records.

Michael, have you submitted the CLA?



On Tue, Dec 17, 2013 at 2:17 PM, Dave Cheney <dave@cheney.net> wrote:

> Can someone please do a A+C for Michael. His details are already on file
> for gccgo contributions.
>
> > On 18 Dec 2013, at 9:14, khr@golang.org wrote:
> >
> >> On 2013/12/17 03:12:54, mwhudson wrote:
> >> Ping?
> >
> > LGTM
> >
> > https://codereview.appspot.com/26570046/
>
> --
>
> ---
> You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>

[minux1, 2013-12-17 22:23:32]

On Tue, Dec 17, 2013 at 5:20 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> I don't see him in our CLA records.
>
It seems he signed a corporate CLA, https://codereview.appspot.com/28760043

[iant, 2013-12-17 22:25:09]

That is correct.  His work is covered by Linaro's CLA.

Ian

On Tue, Dec 17, 2013 at 2:23 PM, minux <minux.ma@gmail.com> wrote:
>
> On Tue, Dec 17, 2013 at 5:20 PM, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
>>
>> I don't see him in our CLA records.
>
> It seems he signed a corporate CLA, https://codereview.appspot.com/28760043

[bradfitz, 2013-12-17 22:41:47]

Okay, I found it.  It wasn't in the normal place yet.

Sent off https://codereview.appspot.com/43670043




On Tue, Dec 17, 2013 at 2:25 PM, Ian Lance Taylor <iant@golang.org> wrote:

> That is correct.  His work is covered by Linaro's CLA.
>
> Ian
>
> On Tue, Dec 17, 2013 at 2:23 PM, minux <minux.ma@gmail.com> wrote:
> >
> > On Tue, Dec 17, 2013 at 5:20 PM, Brad Fitzpatrick <bradfitz@golang.org>
> > wrote:
> >>
> >> I don't see him in our CLA records.
> >
> > It seems he signed a corporate CLA,
> https://codereview.appspot.com/28760043
>

[bradfitz, 2013-12-17 22:49:54]

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

reflect: Add tests for Call with functions taking and returning structs.

gccgo has problems using reflect.Call with functions that take and
return structs with no members.  Prior to fixing that problem there, I
thought it sensible to add some tests of this situation.

Update issue 6761

First contribution to Go, apologies in advance if I'm doing it wrong.

R=golang-dev, dave, minux.ma, iant, khr, bradfitz
CC=golang-dev
https://codereview.appspot.com/26570046

Committer: Brad Fitzpatrick <bradfitz@golang.org>