code review 181161: cgo: Make constants #define'd in C available to Go (as...

cgo: Make constants #define'd in C available to Go (as consts)

  Fixes issue 435

[dho, 2010-01-07 01:20:12]

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

I'd like you to review the following change.

[rsc, 2010-01-08 08:12:55]

http://codereview.appspot.com/181161/diff/1001/11
File src/cmd/cgo/gcc.go (right):

http://codereview.appspot.com/181161/diff/1001/11#newcode30
src/cmd/cgo/gcc.go:30: if len(line) < 9 || line[0:7] != "#define" {
I'd be inclined to use f := strings.Fields and then
val = strings.Join(f[2:], " ")

Should check that key is in p.Crefs.

Once you know key is in p.Crefs, should check that val
parses as a Go expression (import go/parser and call ParseExpr).  Can print an
error if not.

Alternately, we could figure out how to get the C type
and value of the expression somehow.

[dho, 2010-01-08 13:12:09]

2010/1/8  <rsc@golang.org>:
>
> http://codereview.appspot.com/181161/diff/1001/11
> File src/cmd/cgo/gcc.go (right):
>
> http://codereview.appspot.com/181161/diff/1001/11#newcode30
> src/cmd/cgo/gcc.go:30: if len(line) < 9 || line[0:7] != "#define" {
> I'd be inclined to use f := strings.Fields and then
> val = strings.Join(f[2:], " ")

The reason I did not want to do this is because of whitespace in
#defined string constants. If someone has tabs embedded, for example.
It makes the key/val parsing gross, I do admit, but it keeps any later
(non-space) whitespace in a defined constant correct.

> Should check that key is in p.Crefs.

I didn't want to do that at that point in the code because p.Crefs is
an array, and we already iterate over it a bunch of times, so I added
it to the iterator under this bit. If you'd rather have me do it up
there for reducing memory footprint, I'll move it.

> Once you know key is in p.Crefs, should check that val
> parses as a Go expression (import go/parser and call ParseExpr).  Can
> print an error if not.

Done. I also made it exit if ParseExpr returns err != nil since that
won't end up generating valid code anyway.

> Alternately, we could figure out how to get the C type
> and value of the expression somehow.

I'll take a look at this at some point soon. We could certainly do it
again by parsing error messages from gcc, but that seems prone to
error. I'd also like to support wrapping things like:

#define FOO(x, y) x>y?x:y

into perhaps a C wrapper function. But that requires a lot more
thought than I'd like to give it immediately.

> http://codereview.appspot.com/181161
>

PTAL

[dho, 2010-01-11 20:00:37]

Ping? Additional thoughts on these points?

[rsc, 2010-01-11 20:58:54]

LGTM

[rsc, 2010-01-11 21:05:29]

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

	cgo: Make constants #define'd in C available to Go (as consts)

  Fixes issue 435

R=rsc
CC=golang-dev
http://codereview.appspot.com/181161

Committer: Russ Cox <rsc@golang.org>