Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(229)

Issue 114990044: code review 114990044: undo CL 109640045 / f97fb06525e5 (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 8 months ago by minux
Modified:
10 years, 8 months ago
Reviewers:
dave
CC:
dave_cheney.net, golang-codereviews
Visibility:
Public.

Description

undo CL 109640045 / f97fb06525e5 Breaks build for FreeBSD. Probably clang related? ««« original CL description 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> »»»

Patch Set 1 #

Patch Set 2 : diff -r f97fb06525e5 https://code.google.com/p/go #

Patch Set 3 : diff -r f97fb06525e5 https://code.google.com/p/go #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2 lines, -22 lines) Patch
R misc/cgo/test/empty.go View 1 1 chunk +0 lines, -18 lines 0 comments Download
M src/cmd/cgo/out.go View 1 2 chunks +2 lines, -4 lines 0 comments Download

Messages

Total messages: 3
minux
Hello dave@cheney.net (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
10 years, 8 months ago (2014-07-18 06:59:15 UTC) #1
minux
*** Submitted as https://code.google.com/p/go/source/detail?r=99630144d7fc *** undo CL 109640045 / f97fb06525e5 Breaks build for FreeBSD. Probably ...
10 years, 8 months ago (2014-07-18 06:59:57 UTC) #2
dave_cheney.net
10 years, 8 months ago (2014-07-18 07:02:49 UTC) #3
LGTM. Thanks



> On 18 Jul 2014, at 16:59, minux@golang.org wrote:
> 
> Reviewers: dfc,
> 
> Message:
> Hello dave@cheney.net (cc: golang-codereviews@googlegroups.com),
> 
> I'd like you to review this change to
> https://code.google.com/p/go
> 
> 
> Description:
> undo CL 109640045 / f97fb06525e5
> 
> Breaks build for FreeBSD. Probably clang related?
> 
> ««« original CL description
> 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>
> »»»
> 
> Please review this at https://codereview.appspot.com/114990044/
> 
> Affected files (+2, -22 lines):
>  R misc/cgo/test/empty.go
>  M src/cmd/cgo/out.go
> 
> 
> Index: misc/cgo/test/empty.go
> ===================================================================
> deleted file mode 100644
> --- a/misc/cgo/test/empty.go
> +++ /dev/null
> @@ -1,18 +0,0 @@
> -// Copyright 2014 The Go Authors.  All rights reserved.
> -// Use of this source code is governed by a BSD-style
> -// license that can be found in the LICENSE file.
> -
> -package cgotest
> -
> -/*
> -#cgo CFLAGS: -Werror=unused-variable
> -void funcWithoutAnyParams() {}
> -*/
> -import "C"
> -
> -// Only test whether this can be compiled, unused
> -// variable (e.g. empty gcc strut) could cause
> -// warning/error under stricter CFLAGS.
> -func testEmptyGccStruct() {
> -    C.funcWithoutAnyParams()
> -}
> Index: src/cmd/cgo/out.go
> ===================================================================
> --- a/src/cmd/cgo/out.go
> +++ b/src/cmd/cgo/out.go
> @@ -517,7 +517,7 @@
>        return
>    }
> 
> -    ctype, offset := p.structType(n)
> +    ctype, _ := p.structType(n)
> 
>    // Gcc wrapper unpacks the C argument struct
>    // and calls the actual C function.
> @@ -530,9 +530,7 @@
>    // We're trying to write a gcc struct that matches 6c/8c/5c's layout.
>    // Use packed attribute to force no padding in this struct in case
>    // gcc has different packing requirements.
> -    if offset != 0 {
> -        fmt.Fprintf(fgcc, "\t%s %v *a = v;\n", ctype, p.packedAttribute())
> -    }
> +    fmt.Fprintf(fgcc, "\t%s %v *a = v;\n", ctype, p.packedAttribute())
>    fmt.Fprintf(fgcc, "\t")
>    if t := n.FuncType.Result; t != nil {
>        fmt.Fprintf(fgcc, "a->r = ")
> 
> 
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b