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

Issue 15070043: code review 15070043: cmd/cgo: stop using compiler error message text to anal... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 6 months ago by rsc
Modified:
10 years, 5 months ago
Reviewers:
r, iant
CC:
golang-dev, r, james4k, iant
Visibility:
Public.

Description

cmd/cgo: stop using compiler error message text to analyze C names The old approach to determining whether "name" was a type, constant, or expression was to compile the C program name; and scan the errors and warnings generated by the compiler. This requires looking for specific substrings in the errors and warnings, which ties the implementation to specific compiler versions. As compilers change their errors or drop warnings, cgo breaks. This happens slowly but it does happen. Clang in particular (now required on OS X) has a significant churn rate. The new approach compiles a slightly more complex program that is either valid C or not valid C depending on what kind of thing "name" is. It uses only the presence or absence of an error message on a particular line, not the error text itself. The program is: // error if and only if name is undeclared void f1(void) { typeof(name) *x; } // error if and only if name is not a type void f2(void) { name *x; } // error if and only if name is not an integer constant void f3(void) { enum { x = (name)*1 }; } I had not been planning to do this until Go 1.3, because it is a non-trivial change, but it fixes a real Xcode 5 problem in Go 1.2, and the new code is easier to understand than the old code. It should be significantly more robust. Fixes issue 6596. Fixes issue 6612.

Patch Set 1 #

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

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

Total comments: 2

Patch Set 4 : diff -r e7ddf900fd19 https://code.google.com/p/go/ #

Patch Set 5 : diff -r e7ddf900fd19 https://code.google.com/p/go/ #

Total comments: 4

Patch Set 6 : diff -r 95336afd420c https://code.google.com/p/go/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+229 lines, -149 lines) Patch
M misc/cgo/test/cgo_test.go View 1 1 chunk +1 line, -0 lines 0 comments Download
A misc/cgo/test/issue6612.go View 1 2 3 1 chunk +93 lines, -0 lines 0 comments Download
M src/cmd/cgo/doc.go View 1 2 3 4 1 chunk +21 lines, -20 lines 0 comments Download
M src/cmd/cgo/gcc.go View 1 2 3 4 4 chunks +114 lines, -129 lines 0 comments Download

Messages

Total messages: 15
rsc
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go/
10 years, 6 months ago (2013-10-18 17:57:02 UTC) #1
r
doc.go will need updating too
10 years, 6 months ago (2013-10-18 18:06:15 UTC) #2
r
that's clever. https://codereview.appspot.com/15070043/diff/50001/misc/cgo/test/issue6612.go File misc/cgo/test/issue6612.go (right): https://codereview.appspot.com/15070043/diff/50001/misc/cgo/test/issue6612.go#newcode79 misc/cgo/test/issue6612.go:79: * nice formatting, gofmt
10 years, 6 months ago (2013-10-18 18:07:55 UTC) #3
rsc
Hello golang-dev@googlegroups.com, r@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
10 years, 6 months ago (2013-10-18 18:16:54 UTC) #4
rsc
I believe doc.go is vague enough about the details here that it doesn't need updating.
10 years, 6 months ago (2013-10-18 18:17:51 UTC) #5
r
The code is fine but this needs doc.go too.
10 years, 6 months ago (2013-10-18 18:18:24 UTC) #6
r
Really? It's got examples and text that aren't correct now if I understand things properly ...
10 years, 6 months ago (2013-10-18 18:20:07 UTC) #7
r
I'm referring to the implementation section, not the exposed manual.
10 years, 6 months ago (2013-10-18 18:21:37 UTC) #8
james4k
On 2013/10/18 17:57:02, rsc wrote: > Hello mailto:golang-dev@googlegroups.com, > > I'd like you to review ...
10 years, 6 months ago (2013-10-18 18:41:41 UTC) #9
rsc
On Fri, Oct 18, 2013 at 2:21 PM, Rob Pike <r@golang.org> wrote: > I'm referring ...
10 years, 6 months ago (2013-10-18 19:02:06 UTC) #10
rsc
Hello golang-dev@googlegroups.com, r@golang.org, james@james4k.com (cc: golang-dev@googlegroups.com), Please take another look.
10 years, 6 months ago (2013-10-18 19:13:50 UTC) #11
r
LGTM
10 years, 6 months ago (2013-10-18 19:49:58 UTC) #12
iant
LGTM Nice. https://codereview.appspot.com/15070043/diff/150001/src/cmd/cgo/doc.go File src/cmd/cgo/doc.go (right): https://codereview.appspot.com/15070043/diff/150001/src/cmd/cgo/doc.go#newcode273 src/cmd/cgo/doc.go:273: void __cgo_f_xxx_1(void) { typeof(foo) *__cgo_undefined__; } typeof ...
10 years, 6 months ago (2013-10-18 19:55:59 UTC) #13
rsc
*** Submitted as https://code.google.com/p/go/source/detail?r=90a628ac54ed *** cmd/cgo: stop using compiler error message text to analyze C ...
10 years, 6 months ago (2013-10-18 19:56:29 UTC) #14
rsc
10 years, 6 months ago (2013-10-18 20:02:41 UTC) #15
thanks, ian. i didn't get that before submitting, so i'll apply those in a
follow up.
for what it's worth, we've always used typeof elsewhere in the generated
programs, so no one must be using -std=c11.
i'll change all the instances though.
Sign in to reply to this message.

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