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

Issue 9835047: code review 9835047: cmd/cgo: Add support for C function pointers (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 5 months ago by Hierro
Modified:
4 years, 3 months ago
Reviewers:
rsc, iant
CC:
golang-dev, minux1, albert.strasheim, rsc, iant
Visibility:
Public.

Description

cmd/cgo: Add support for C function pointers * Add a new kind of Name, "fpvar" which stands for function pointer variable * When walking the AST, find functions used as expressions and create a new Name object for them * Track functions which are only used in expr contexts, and avoid generating bridge code for them

Patch Set 1 #

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

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

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

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

Total comments: 4

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

Total comments: 1

Patch Set 7 : diff -r d881cb1ffc14 https://code.google.com/p/go #

Unified diffs Side-by-side diffs Delta from patch set Stats (+148 lines, -19 lines) Patch
M misc/cgo/test/cgo_test.go View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
A misc/cgo/test/fpvar.go View 1 2 3 4 5 6 1 chunk +50 lines, -0 lines 0 comments Download
M src/cmd/cgo/doc.go View 1 2 3 4 5 1 chunk +27 lines, -0 lines 0 comments Download
M src/cmd/cgo/gcc.go View 1 2 3 4 5 4 chunks +50 lines, -13 lines 0 comments Download
M src/cmd/cgo/main.go View 1 1 chunk +6 lines, -1 line 0 comments Download
M src/cmd/cgo/out.go View 1 2 3 4 5 2 chunks +14 lines, -5 lines 0 comments Download

Messages

Total messages: 16
Hierro
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
4 years, 5 months ago (2013-05-31 11:09:47 UTC) #1
Hierro
Just wanted to add that all the tests in misc/cgo/tests continue to pass correctly after ...
4 years, 5 months ago (2013-05-31 11:10:42 UTC) #2
minux1
On Fri, May 31, 2013 at 7:10 PM, <alberto.garcia.hierro@gmail.com> wrote: > Just wanted to add ...
4 years, 5 months ago (2013-05-31 11:40:38 UTC) #3
albert.strasheim
Fixes issue 3364.
4 years, 5 months ago (2013-05-31 12:09:00 UTC) #4
albert.strasheim
FWIW, I was hoping this code might work now: package jvm //#cgo CFLAGS: -I/usr/java/jdk1.7.0_21/include //#cgo ...
4 years, 5 months ago (2013-05-31 12:31:52 UTC) #5
Hierro
On 2013/05/31 12:31:52, albert.strasheim wrote: > FWIW, I was hoping this code might work now: ...
4 years, 5 months ago (2013-05-31 12:56:21 UTC) #6
Hierro
Ping
4 years, 4 months ago (2013-07-18 21:46:25 UTC) #7
rsc
This looks good, and it's great that misc/cgo/test continues to pass, but it already passes ...
4 years, 3 months ago (2013-08-01 18:15:23 UTC) #8
Hierro
On 2013/08/01 18:15:23, rsc wrote: > This looks good, and it's great that misc/cgo/test continues ...
4 years, 3 months ago (2013-08-02 12:22:45 UTC) #9
iant
https://codereview.appspot.com/9835047/diff/19001/misc/cgo/test/fpvar.go File misc/cgo/test/fpvar.go (right): https://codereview.appspot.com/9835047/diff/19001/misc/cgo/test/fpvar.go#newcode1 misc/cgo/test/fpvar.go:1: package cgotest This new file needs a 2013 copyright ...
4 years, 3 months ago (2013-08-07 14:19:46 UTC) #10
iant
It seems appropriate to add a sentence or two to doc.go about this, e.g., in ...
4 years, 3 months ago (2013-08-07 14:27:58 UTC) #11
Hierro
Hi Ian, Thanks for the review. I've added the changes you suggested, as well as ...
4 years, 3 months ago (2013-08-07 17:12:39 UTC) #12
iant
LGTM Please wait for rsc. https://codereview.appspot.com/9835047/diff/28001/misc/cgo/test/fpvar.go File misc/cgo/test/fpvar.go (right): https://codereview.appspot.com/9835047/diff/28001/misc/cgo/test/fpvar.go#newcode5 misc/cgo/test/fpvar.go:5: // This file contains ...
4 years, 3 months ago (2013-08-07 17:22:02 UTC) #13
Hierro
On 2013/08/07 17:22:02, iant wrote: > LGTM > > Please wait for rsc. > > ...
4 years, 3 months ago (2013-08-07 20:30:08 UTC) #14
rsc
LGTM
4 years, 3 months ago (2013-08-13 16:42:10 UTC) #15
rsc
4 years, 3 months ago (2013-08-13 16:42:24 UTC) #16
*** Submitted as https://code.google.com/p/go/source/detail?r=abd44d99e4cf ***

cmd/cgo: Add support for C function pointers

* Add a new kind of Name, "fpvar" which stands for function pointer variable
* When walking the AST, find functions used as expressions and create a new Name
object for them
* Track functions which are only used in expr contexts, and avoid generating
bridge code for them

R=golang-dev, minux.ma, fullung, rsc, iant
CC=golang-dev
https://codereview.appspot.com/9835047

Committer: Russ Cox <rsc@golang.org>
Sign in to reply to this message.

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