code review 9574043: cgo: do not output empty struct (issue 9574043)

[alex.b...@gmail.com]

Reviewers: golang-dev1,

Message:
Hello golan...@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/


Description:
cgo: do not output empty struct

It also includes regenerated pkg/runtime/defs_windows_*.h

Please review this at https://codereview.appspot.com/9574043/

Affected files:
M src/cmd/cgo/godefs.go
M src/pkg/runtime/defs_windows_386.h
M src/pkg/runtime/defs_windows_amd64.h

[ia...@golang.org]

What empty struct is avoided in the regenerated output files?

https://codereview.appspot.com/9574043/

[alex.b...@gmail.com]

These are the diffs for defs_windows_386.h (and defs_windows_amd64.h)
files generated with current cgo command and proposed changed one:

# diff defs_windows_amd64.h
~/cl9574043/src/pkg/runtime/defs_windows_amd64.h

64,65d63

< struct FloatingSaveArea {

< };

# diff defs_windows_386.h ~/cl9574043/src/pkg/runtime/defs_windows_386.h

74,75d73

< struct M128a {

< };

#


It seems FloatingSaveArea and M128a are ARCH specific. They are not used
directly by our code, but they are embeded in other structs. They are
present in defs_windows.go, because cgo doesn't do recursion to discover
embeded structs.

I suppose, FloatingSaveArea and M128a can be moved into ARCH specific
file like defs_windows_386.go and defs_windows_amd64.go, but I think it
just complicates things more.

I can just abandon this CL altogether. But I need more windows C structs
for net poller, and it bothers me that auto-generation does not work.

Alex

https://codereview.appspot.com/9574043/

[ia...@golang.org]

Are the empty structs a problem?


https://codereview.appspot.com/9574043/

[alex.b...@gmail.com]

On 2013/05/23 04:40:47, iant wrote:
> Are the empty structs a problem?

Yes. These fail to compile.

Alex

https://codereview.appspot.com/9574043/

[ia...@golang.org]

On 2013/05/23 04:41:32, brainman wrote:
> On 2013/05/23 04:40:47, iant wrote:
> > Are the empty structs a problem?

> Yes. These fail to compile.

GCC permits empty structs, although I don't think they are in standard
C. Are they failing with a non-GCC compiler?

https://codereview.appspot.com/9574043/

[ia...@golang.org]

On a separate topic, the changes to the defs files are large and in
general have nothing to do with the change to cgo. Let's separate this
CL from the CL that regenerates those files.

https://codereview.appspot.com/9574043/

[alex.b...@gmail.com]

On 2013/05/23 04:50:42, iant wrote:

> GCC permits empty structs, although I don't think they are in standard
C. Are
> they failing with a non-GCC compiler?

Yes. They fail with 8c:

C:\go\root\src\pkg\runtime>go tool cgo -cdefs defs_windows.go >
defs_windows_386.h

C:\go\root\src\pkg\runtime>go install -v
runtime
# runtime
C:\DOCUME~1\brainman\LOCALS~1\Temp\go-build165623463\runtime\_obj\/defs_GOOS_GOARCH.h:75
c:\go\root\src\pkg\runtime\callback_windows_386.c:8 syntax error,
lastname: M128a

Alex

https://codereview.appspot.com/9574043/

[alex.b...@gmail.com]

On 2013/05/23 04:51:19, iant wrote:
> On a separate topic, the changes to the defs files are large and in
general have
> nothing to do with the change to cgo. Let's separate this CL from the
CL that
> regenerates those files.

Fair enough. I will remove them. But let's decide what to do first.

Alex

https://codereview.appspot.com/9574043/

[ia...@golang.org]

On 2013/05/23 04:56:05, brainman wrote:
> On 2013/05/23 04:51:19, iant wrote:
> > On a separate topic, the changes to the defs files are large and in
general
> have
> > nothing to do with the change to cgo. Let's separate this CL from
the CL that
> > regenerates those files.

> Fair enough. I will remove them. But let's decide what to do first.

I'm fine with omitting empty structs from the generated C code. They
are unlikely to be useful.

https://codereview.appspot.com/9574043/

[alex.b...@gmail.com]

Hello golan...@googlegroups.com, ia...@golang.org (cc:
golan...@googlegroups.com),

Please take another look.


https://codereview.appspot.com/9574043/

[alex.b...@gmail.com]

On 2013/05/23 04:51:19, iant wrote:

> ... Let's separate this CL from the CL that
> regenerates those files.

Done https://codereview.appspot.com/9679046/

Alex

https://codereview.appspot.com/9574043/

[ia...@golang.org]

LGTM

Before submitting please change CL description to

cmd/cgo: do not output empty struct for -cdefs


https://codereview.appspot.com/9574043/

[alex.b...@gmail.com]

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

cmd/cgo: do not output empty struct for -cdefs

R=golang-dev, iant
CC=golang-dev
https://codereview.appspot.com/9574043


https://codereview.appspot.com/9574043/