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

Issue 8248043: code review 8248043: cmd/go: Add support for including C++ files in packages (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 12 months ago by Hierro
Modified:
10 years, 9 months ago
Reviewers:
binet, r, iant
CC:
iant, r, bradfitz, gobot, golang-dev, minux1, remyoudompheng, binet
Visibility:
Public.

Description

cmd/go: Add support for including C++ files in packages * Add a CXXFiles field to Package, which includes .cc, .cpp and .cxx files. * CXXFiles are compiled using g++, which can be overridden using the CXX environment variable. * Include .hh, .hpp and .hxx files in HFiles. * Add support for CPPFLAGS (used for both C and C++) and CXXFLAGS (used only for C++) in cgo directive. * Changed pkg-config cgo directive to modify CPPFLAGS rather than CFLAGS, so both C and C++ files get any flag returned by pkg-config --cflags. Fixes issue 1476.

Patch Set 1 #

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

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

Total comments: 7

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

Total comments: 1

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

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

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

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

Total comments: 10

Patch Set 9 : diff -r bb92bbe623fa https://code.google.com/p/go #

Patch Set 10 : diff -r bb92bbe623fa https://code.google.com/p/go #

Patch Set 11 : diff -r fd791d51e476 https://code.google.com/p/go #

Total comments: 8

Patch Set 12 : diff -r fd791d51e476 https://code.google.com/p/go #

Patch Set 13 : diff -r fd791d51e476 https://code.google.com/p/go #

Total comments: 1

Patch Set 14 : diff -r e86ab7e59e50 https://code.google.com/p/go #

Unified diffs Side-by-side diffs Delta from patch set Stats (+142 lines, -42 lines) Patch
M src/cmd/cgo/doc.go View 1 2 3 4 5 6 7 8 3 chunks +3 lines, -3 lines 0 comments Download
M src/cmd/go/build.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 chunks +94 lines, -28 lines 0 comments Download
M src/cmd/go/doc.go View 1 2 3 1 chunk +4 lines, -1 line 0 comments Download
M src/cmd/go/list.go View 1 2 3 4 5 1 chunk +4 lines, -1 line 0 comments Download
M src/cmd/go/pkg.go View 1 2 3 4 5 6 7 8 9 10 11 5 chunks +14 lines, -2 lines 0 comments Download
M src/cmd/godoc/index.go View 1 2 3 1 chunk +6 lines, -0 lines 0 comments Download
M src/pkg/go/build/build.go View 1 2 3 4 5 6 7 8 5 chunks +17 lines, -7 lines 0 comments Download

Messages

Total messages: 39
Hierro
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
10 years, 12 months ago (2013-04-02 02:37:01 UTC) #1
bradfitz
Sorry, too late for Go 1.1. Please bring this up again once Go 1.1 is ...
10 years, 12 months ago (2013-04-02 02:40:18 UTC) #2
Hierro
On 2013/04/02 02:40:18, bradfitz wrote: > Sorry, too late for Go 1.1. > > Please ...
10 years, 12 months ago (2013-04-02 03:00:44 UTC) #3
bradfitz
Set a reminder to poll https://code.google.com/p/go/downloads/list in two weeks. If go1.1 isn't there, repeat. :-) ...
10 years, 12 months ago (2013-04-02 03:20:30 UTC) #4
remyoudompheng
I'm pretty sure it used to not work so simply in the past, the linker ...
10 years, 12 months ago (2013-04-02 05:28:17 UTC) #5
Hierro
On 2013/04/02 03:20:30, bradfitz wrote: > Set a reminder to poll https://code.google.com/p/go/downloads/list in two > ...
10 years, 12 months ago (2013-04-02 13:44:46 UTC) #6
minux1
On Tue, Apr 2, 2013 at 9:44 PM, <alberto.garcia.hierro@gmail.com> wrote: > I was under the ...
10 years, 12 months ago (2013-04-02 13:50:54 UTC) #7
Hierro
message: On 2013/04/02 13:50:54, minux wrote: > Go 1.1 is feature frozen now. > > ...
10 years, 12 months ago (2013-04-05 07:01:38 UTC) #8
binet
minor cosmetics suggestions (CPPFiles -> CXXFiles) and add missing popular extensions (.cxx, .hxx, .hh and ...
10 years, 12 months ago (2013-04-05 12:20:33 UTC) #9
Hierro
I've implemented binet's suggestions (except for the .C extension, since it could cause problems in ...
10 years, 11 months ago (2013-04-07 12:26:10 UTC) #10
binet
On 2013/04/07 12:26:10, Hierro wrote: > I've implemented binet's suggestions (except for the .C extension, ...
10 years, 11 months ago (2013-04-07 12:30:44 UTC) #11
binet
hi, just one last minor comment: the new CgoCPPFLAGS and CgoCXXFLAGS fields are missing from ...
10 years, 11 months ago (2013-04-07 12:53:24 UTC) #12
Hierro
Hi Sebastien, Nice catch :-). I've just updated the patchset and also added a few ...
10 years, 11 months ago (2013-04-07 16:37:40 UTC) #13
Hierro
I've just updated this CL with the current tip, since there were some commits affecting ...
10 years, 11 months ago (2013-04-12 07:52:58 UTC) #14
Hierro
Now that Go 1.1 is out, it would be nice if we could discuss this ...
10 years, 10 months ago (2013-05-14 13:02:56 UTC) #15
Hierro
Hello golang-dev@googlegroups.com, bradfitz@golang.org, remyoudompheng@gmail.com, minux.ma@gmail.com, seb.binet@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
10 years, 10 months ago (2013-05-16 07:42:34 UTC) #16
binet
On 2013/05/16 07:42:34, Hierro wrote: > Hello mailto:golang-dev@googlegroups.com, mailto:bradfitz@golang.org, > mailto:remyoudompheng@gmail.com, mailto:minux.ma@gmail.com, mailto:seb.binet@gmail.com (cc: > ...
10 years, 10 months ago (2013-05-16 07:55:05 UTC) #17
iant
https://codereview.appspot.com/8248043/diff/52001/misc/dist/bindist.go File misc/dist/bindist.go (right): https://codereview.appspot.com/8248043/diff/52001/misc/dist/bindist.go#newcode859 misc/dist/bindist.go:859: "Mingw g++; http://sourceforge.net/projects/mingw/files/Installer/mingw-get-inst/", I don't understand why it matters ...
10 years, 10 months ago (2013-05-22 20:56:21 UTC) #18
Hierro
Hi Ian, Thanks for the review, I've added some comments below. On 2013/05/22 20:56:21, iant ...
10 years, 10 months ago (2013-05-23 16:11:33 UTC) #19
Hierro
Hello golang-dev@googlegroups.com, bradfitz@golang.org, remyoudompheng@gmail.com, minux.ma@gmail.com, seb.binet@gmail.com, iant@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
10 years, 10 months ago (2013-05-23 16:32:10 UTC) #20
gobot
R=rsc (assigned by dsymonds)
10 years, 10 months ago (2013-05-27 00:54:14 UTC) #21
Hierro
On 2013/05/27 00:54:14, gobot wrote: > R=rsc (assigned by dsymonds) Ping
10 years, 10 months ago (2013-05-31 13:54:56 UTC) #22
r
10 years, 10 months ago (2013-05-31 14:07:00 UTC) #23
r
-rsc +iant
10 years, 10 months ago (2013-05-31 14:07:49 UTC) #24
r
Please run hg mail 8248043 to refresh the codereview repository.
10 years, 10 months ago (2013-05-31 14:08:10 UTC) #25
iant
I'm seeing "chunk mismatch" in rietveld. Sorry, but can you upload the CL again?
10 years, 10 months ago (2013-05-31 14:21:29 UTC) #26
Hierro
On 2013/05/31 14:21:29, iant wrote: > I'm seeing "chunk mismatch" in rietveld. Sorry, but can ...
10 years, 10 months ago (2013-05-31 14:57:01 UTC) #27
r
https://codereview.appspot.com/8248043/diff/80003/src/cmd/cgo/doc.go File src/cmd/cgo/doc.go (right): https://codereview.appspot.com/8248043/diff/80003/src/cmd/cgo/doc.go#newcode27 src/cmd/cgo/doc.go:27: CPPFLAGS, CFLAGS, CXXFLAGS and LDFLAGS may be defined with ...
10 years, 10 months ago (2013-05-31 15:42:29 UTC) #28
iant
https://codereview.appspot.com/8248043/diff/80003/src/cmd/dist/build.c File src/cmd/dist/build.c (right): https://codereview.appspot.com/8248043/diff/80003/src/cmd/dist/build.c#newcode889 src/cmd/dist/build.c:889: if(!hassuffix(files.p[i], ".c") && !hassuffix(files.p[i], ".cc") && !hassuffix(files.p[i], ".cpp") && ...
10 years, 10 months ago (2013-05-31 15:44:05 UTC) #29
Hierro
Hi Ian, Thanks for the review. Comments are below. I've also uploaded a new patchset ...
10 years, 10 months ago (2013-05-31 16:50:25 UTC) #30
Hierro
On 2013/05/31 15:42:29, r wrote: > https://codereview.appspot.com/8248043/diff/80003/src/cmd/cgo/doc.go > File src/cmd/cgo/doc.go (right): > > https://codereview.appspot.com/8248043/diff/80003/src/cmd/cgo/doc.go#newcode27 > ...
10 years, 10 months ago (2013-05-31 17:17:14 UTC) #31
iant
LGTM Please wait for r. https://codereview.appspot.com/8248043/diff/102001/src/cmd/go/build.go File src/cmd/go/build.go (right): https://codereview.appspot.com/8248043/diff/102001/src/cmd/go/build.go#newcode748 src/cmd/go/build.go:748: return fmt.Errorf("Can't build package ...
10 years, 10 months ago (2013-05-31 18:16:26 UTC) #32
r
LGTM you're right - it seems there is a gap in the path and filepath ...
10 years, 10 months ago (2013-05-31 18:20:16 UTC) #33
Hierro
On 2013/05/31 18:16:26, iant wrote: > LGTM > > Please wait for r. > > ...
10 years, 10 months ago (2013-05-31 18:28:43 UTC) #34
iant
*** Submitted as https://code.google.com/p/go/source/detail?r=24596e5bca7d *** cmd/go: Add support for including C++ files in packages * ...
10 years, 10 months ago (2013-05-31 18:33:45 UTC) #35
Hierro
On 2013/05/31 18:20:16, r wrote: > LGTM > > you're right - it seems there ...
10 years, 10 months ago (2013-05-31 18:54:04 UTC) #36
minux1
Should we update doc/go1.2.txt? On Sat, Jun 1, 2013 at 2:33 AM, <iant@golang.org> wrote: > ...
10 years, 10 months ago (2013-05-31 18:54:38 UTC) #37
r
On Fri, May 31, 2013 at 6:54 PM, minux <minux.ma@gmail.com> wrote: > Should we update ...
10 years, 10 months ago (2013-05-31 19:02:16 UTC) #38
Hierro
10 years, 9 months ago (2013-06-20 11:35:52 UTC) #39
Message was sent while issue was closed.
*** Abandoned ***
Sign in to reply to this message.

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