code review 109640045: cgo: disable inappropriate warnings when the gcc struct...

cmd/cgo: disable inappropriate warnings when the gcc struct is empty

package main
//#cgo CFLAGS: -Wall
//void test() {}
import "C"
func main() {
    C.test()
}

This code will cause gcc issuing warnings about unused variable.

This commit use offset of the second return value of
Packages.structType to detect whether the gcc struct is empty,
and if it's directly invoke the C function instead of writing an
unused code.

[snyh, 2014-07-10 14:20:21]

Hello golang-codereviews@googlegroups.com,

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

[iant, 2014-07-10 15:41:41]

Please add a test to misc/cgo/test.

[minux, 2014-07-10 16:31:51]

On Thu, Jul 10, 2014 at 11:41 AM, <iant@golang.org> wrote:

> Please add a test to misc/cgo/test.
>
it seems we just need to add -Werror=unused-variable to misc/cgo/test's
CFLAGS.
no new file or function is needed. there are already 3 instances in
callback.go

[snyh, 2014-07-11 03:23:09]

If there really need a test file, what it should be do?
I'm not familiar with golang source tree. If your can give me some help
information, I will be glad to write one.

like minux says,  this warnings can be produce easily. If we need catch the
warnings,  only a go file is not sufficient.
Is it appropriate add a shell file to detect this error?


On Fri, Jul 11, 2014 at 12:31 AM, minux <minux@golang.org> wrote:

>
> On Thu, Jul 10, 2014 at 11:41 AM, <iant@golang.org> wrote:
>
>> Please add a test to misc/cgo/test.
>>
> it seems we just need to add -Werror=unused-variable to misc/cgo/test's
> CFLAGS.
> no new file or function is needed. there are already 3 instances in
> callback.go
>

[minux, 2014-07-11 03:31:12]

On 2014/07/11 03:23:09, snyh wrote:
> like minux says,  this warnings can be produce easily. If we need catch the
> warnings,  only a go file is not sufficient.
> Is it appropriate add a shell file to detect this error?

i imagine something like this is sufficient as a test.

diff -r c0a68bcf19ae misc/cgo/test/basic.go
--- a/misc/cgo/test/basic.go	Fri Jul 11 11:02:02 2014 +1000
+++ b/misc/cgo/test/basic.go	Thu Jul 10 23:29:38 2014 -0400
@@ -7,6 +7,7 @@
 package cgotest
 
 /*
+#cgo CFLAGS: -Werror=unused-variable
 #include <stdio.h>
 #include <stdlib.h>
 #include <sys/stat.h>

You might want to add some comment and/or create a new file in misc/cgo/test
for this purpose.

[snyh, 2014-07-11 09:10:11]

On 2014/07/10 15:41:41, iant wrote:
> Please add a test to misc/cgo/test.

With the help of minux, I have added it.
Please take some time to review it.

[dave_cheney.net, 2014-07-16 10:19:31]

LGTM. Thank you for adding the test.

Please remove the asterisks' in the description, we don't use markdown for
commit messages.

[dave_cheney.net, 2014-07-16 10:32:13]

Ian, do you have any further comments ? Otherwise, would you please submit
this.

On Wed, Jul 16, 2014 at 8:19 PM,  <dave@cheney.net> wrote:
> LGTM. Thank you for adding the test.
>
> Please remove the asterisks' in the description, we don't use markdown
> for commit messages.
>
>
> https://codereview.appspot.com/109640045/

[iant, 2014-07-16 16:12:39]

https://codereview.appspot.com/109640045/diff/80001/misc/cgo/test/issue109640...
File misc/cgo/test/issue109640045.go (right):

https://codereview.appspot.com/109640045/diff/80001/misc/cgo/test/issue109640...
misc/cgo/test/issue109640045.go:5: // issue 109640045 fix unused variable
warnings/errors caused
This test name and comment aren't useful.  The issueNNNN that you see in
misc/cgo/test refer to http://golang.org/issue/NNNN.  This number is a reference
to codereview.appspot.com, and that isn't useful.  If there is no actual issue
for this, please name this something like empty.go and update the comment and
function name.  Thanks.

[minux, 2014-07-16 16:15:52]

Also, please change the description to start with "cmd/cgo:"
rather than "cgo:"

PS: if you want, you can file an issue at golang.org/issue/new,
and then use that number in your test file and function name.
And if you choose to do this, please add:
Fixes issue NNNN.
at the end of the description (where NNNN is the issue number).

[snyh, 2014-07-17 01:08:03]

On 2014/07/16 16:15:52, minux wrote:
> Also, please change the description to start with "cmd/cgo:"
> rather than "cgo:"
> 
> PS: if you want, you can file an issue at golang.org/issue/new,
> and then use that number in your test file and function name.
> And if you choose to do this, please add:
> Fixes issue NNNN.
> at the end of the description (where NNNN is the issue number).

minux, iant:
Thanks for your patience.

[minux, 2014-07-17 05:13:35]

LGTM.

Have you signed the CLA (see http://golang.org/doc/contribute.html#copyright)?
I searched for your email, but didn't find it.

Please notify me after you've signed the CLA. I will take care
of the A+C change and then submit this CL.

Thanks!

[minux, 2014-07-18 06:47:24]

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

cmd/cgo: disable inappropriate warnings when the gcc struct is empty

package main
//#cgo CFLAGS: -Wall
//void test() {}
import "C"
func main() {
    C.test()
}

This code will cause gcc issuing warnings about unused variable.

This commit use offset of the second return value of
Packages.structType to detect whether the gcc struct is empty,
and if it's directly invoke the C function instead of writing an
unused code.

LGTM=dave, minux
R=golang-codereviews, iant, minux, dave
CC=golang-codereviews
https://codereview.appspot.com/109640045

Committer: Shenghou Ma <minux@golang.org>

[gobot, 2014-07-18 06:52:49]

This CL appears to have broken the freebsd-amd64 builder.
See http://build.golang.org/log/28b084a3e502cd20788aebda99b7ec9cf22e9dd8

[dave_cheney.net, 2014-07-18 06:54:11]

Yup, this is real.



> On 18 Jul 2014, at 16:52, gobot@golang.org wrote:
> 
> This CL appears to have broken the freebsd-amd64 builder.
> See http://build.golang.org/log/28b084a3e502cd20788aebda99b7ec9cf22e9dd8
> 
> https://codereview.appspot.com/109640045/

[minux, 2014-07-18 06:58:04]

On Fri, Jul 18, 2014 at 2:54 AM, Dave Cheney <dave@cheney.net> wrote:

> Yup, this is real.

will rollback. actually, I forgot that Russ's CL to update C hasn't been
submitted yet, so
I shouldn't have submitted this one anyway.

[minux, 2014-07-18 07:02:55]

Hi snyh,

This change appear to have broken freebsd system, and I have rolled it back.
It's clang related, please do this to reproduce the problem on Linux:
CC=clang go test -v ../misc/cgo/test