code review 103080043: cmd/cgo: for typedef of untagged struct, use typedef na...

cmd/cgo: for typedef of untagged struct, use typedef name in C code

Fixes issue 8148.

[iant, 2014-06-04 03:53:03]

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

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

[rsc, 2014-06-04 03:59:36]

Can we do this only if the struct is unnamed? I am worried about some code
having typedefs and other code not. In that case I want the struct name to
be the rendezvous point.

[cookieo9, 2014-06-04 04:11:49]

On 2014/06/04 03:53:03, iant wrote:
> Hello rsc (cc: mailto:golang-codereviews@googlegroups.com),
> 
> I'd like you to review this change to
> https://code.google.com/p/go

Although this does fix the definitions in _cgo_export.h and _cgo_export.c to use
the original C type, the translated code in *.cgo1.go still refers to a
"_Ctype_struct___0".

This doesn't prevent it from compiling any more, but is a little inconsistent,
and a change from the 1.2 behaviour.

[iant, 2014-06-04 13:15:08]

On Tue, Jun 3, 2014 at 8:59 PM, Russ Cox <rsc@golang.org> wrote:
> Can we do this only if the struct is unnamed? I am worried about some code
> having typedefs and other code not. In that case I want the struct name to
> be the rendezvous point.

This change is within the isStructUnionClass condition, so I believe
it already only applies when the struct is unnamed.

Ian

[iant, 2014-06-04 13:16:42]

On Tue, Jun 3, 2014 at 9:11 PM,  <cookieo9@gmail.com> wrote:
>
> Although this does fix the definitions in _cgo_export.h and
> _cgo_export.c to use the original C type, the translated code in
> *.cgo1.go still refers to a "_Ctype_struct___0".
>
> This doesn't prevent it from compiling any more, but is a little
> inconsistent, and a change from the 1.2 behaviour.

That is an intentional change (which is the source of this problem).
See http://golang.org/issue/7786 .

Ian

[cookieo9, 2014-06-04 14:12:17]

On 2014/06/04 13:16:42, iant wrote:
> On Tue, Jun 3, 2014 at 9:11 PM,  <mailto:cookieo9@gmail.com> wrote:
> >
> > Although this does fix the definitions in _cgo_export.h and
> > _cgo_export.c to use the original C type, the translated code in
> > *.cgo1.go still refers to a "_Ctype_struct___0".
> >
> > This doesn't prevent it from compiling any more, but is a little
> > inconsistent, and a change from the 1.2 behaviour.
> 
> That is an intentional change (which is the source of this problem).
> See http://golang.org/issue/7786 .
> 
> Ian

I had found cause of the original problem by diffing the output of "go tool cgo"
between 1.2.2 and tip. I only brought it up because it was a notable difference
from 1.2.2, I have no problem with it otherwise, since the original code now
compiles.

[cookieo9, 2014-06-04 14:12:31]

https://codereview.appspot.com/103080043/diff/1/misc/cgo/test/issue8148.go
File misc/cgo/test/issue8148.go (right):

https://codereview.appspot.com/103080043/diff/1/misc/cgo/test/issue8148.go#ne...
misc/cgo/test/issue8148.go:12: typedef struct { int i; } T;
The original code had a prototype declaration for the callback.
On my machine the test fails with:
./issue8148.go:16:9: error: implicit declaration of function 'issue8148Callback'
is invalid in C99 [-Werror,-Wimplicit-function-declaration]

Suggest adding:
int issue8148Callback (C.T*);

[cookieo9, 2014-06-04 14:14:18]

On 2014/06/04 14:12:31, cookieo9 wrote:
> https://codereview.appspot.com/103080043/diff/1/misc/cgo/test/issue8148.go
> File misc/cgo/test/issue8148.go (right):
> 
>
https://codereview.appspot.com/103080043/diff/1/misc/cgo/test/issue8148.go#ne...
> misc/cgo/test/issue8148.go:12: typedef struct { int i; } T;
> The original code had a prototype declaration for the callback.
> On my machine the test fails with:
> ./issue8148.go:16:9: error: implicit declaration of function
'issue8148Callback'
> is invalid in C99 [-Werror,-Wimplicit-function-declaration]
> 
> Suggest adding:
> int issue8148Callback (C.T*);
whoops, should be:
int issue8148Callback (T*);

[iant, 2014-06-04 14:26:08]

Thanks.

https://codereview.appspot.com/103080043/diff/1/misc/cgo/test/issue8148.go
File misc/cgo/test/issue8148.go (right):

https://codereview.appspot.com/103080043/diff/1/misc/cgo/test/issue8148.go#ne...
misc/cgo/test/issue8148.go:12: typedef struct { int i; } T;
On 2014/06/04 14:12:31, cookieo9 wrote:
> The original code had a prototype declaration for the callback.
> On my machine the test fails with:
> ./issue8148.go:16:9: error: implicit declaration of function
'issue8148Callback'
> is invalid in C99 [-Werror,-Wimplicit-function-declaration]
> 
> Suggest adding:
> int issue8148Callback (C.T*);

Done.

[cookieo9, 2014-06-04 14:46:04]

On 2014/06/04 14:26:08, iant wrote:
> Thanks.
> 
> https://codereview.appspot.com/103080043/diff/1/misc/cgo/test/issue8148.go
> File misc/cgo/test/issue8148.go (right):
> 
>
https://codereview.appspot.com/103080043/diff/1/misc/cgo/test/issue8148.go#ne...
> misc/cgo/test/issue8148.go:12: typedef struct { int i; } T;
> On 2014/06/04 14:12:31, cookieo9 wrote:
> > The original code had a prototype declaration for the callback.
> > On my machine the test fails with:
> > ./issue8148.go:16:9: error: implicit declaration of function
> 'issue8148Callback'
> > is invalid in C99 [-Werror,-Wimplicit-function-declaration]
> > 
> > Suggest adding:
> > int issue8148Callback (C.T*);
> 
> Done.

Test now passes.

LGTM

[rsc, 2014-06-05 16:40:09]

LGTM

[iant, 2014-06-05 17:34:56]

I realized this might crash when using -godefs or -cdefs, so I tweaked it.

[rsc, 2014-06-05 17:35:22]

LGTM

[iant, 2014-06-05 17:42:07]

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

cmd/cgo: for typedef of untagged struct, use typedef name in C code

Fixes issue 8148.

LGTM=cookieo9, rsc
R=rsc, cookieo9
CC=golang-codereviews
https://codereview.appspot.com/103080043