|
|
Descriptioncmd/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 #
MessagesTotal messages: 39
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
Sorry, too late for Go 1.1. Please bring this up again once Go 1.1 is out and we've branched. On Mon, Apr 1, 2013 at 7:37 PM, <alberto.garcia.hierro@gmail.com> wrote: > Reviewers: golang-dev1, > > Message: > Hello golang-dev@googlegroups.com, > > I'd like you to review this change to > https://code.google.com/p/go > > > Description: > cmd/cgo: Add support for including C++ files in packages > > Add a CPPFiles field to Package, which includes any .cc or .cpp files in > the package > directory. These files are built exactly in the same way than .c files, > since gcc > is smart enough to figure the type of the file by its extension. This > enables go code > to call C++ code (although embedded in a C interface using extern "C" > declared > functions) without using SWIG. Also, this patch would fix issue 1476. > > TODO: Support CXXFLAGS directive in cgo files. > > Please review this at https://codereview.appspot.**com/8248043/<https://codereview.appspot.com/8248... > > Affected files: > M src/cmd/dist/build.c > M src/cmd/go/build.go > M src/cmd/go/doc.go > M src/cmd/go/list.go > M src/cmd/go/pkg.go > M src/cmd/godoc/index.go > M src/pkg/go/build/build.go > > > Index: src/cmd/dist/build.c > ==============================**==============================**======= > --- a/src/cmd/dist/build.c > +++ b/src/cmd/dist/build.c > @@ -539,6 +539,8 @@ > // depsuffix records the allowed suffixes for source files. > char *depsuffix[] = { > ".c", > + ".cc", > + ".cpp", > ".h", > ".s", > ".go", > @@ -883,7 +885,7 @@ > > // Compile the files. > for(i=0; i<files.len; i++) { > - if(!hassuffix(files.p[i], ".c") && !hassuffix(files.p[i], > ".s")) > + if(!hassuffix(files.p[i], ".c") && !hassuffix(files.p[i], > ".cc") && !hassuffix(files.p[i], ".cpp") && !hassuffix(files.p[i], ".s")) > continue; > name = lastelem(files.p[i]); > > Index: src/cmd/go/build.go > ==============================**==============================**======= > --- a/src/cmd/go/build.go > +++ b/src/cmd/go/build.go > @@ -766,11 +766,12 @@ > var gofiles, cfiles, sfiles, objects, cgoObjects []string > gofiles = append(gofiles, a.p.GoFiles...) > cfiles = append(cfiles, a.p.CFiles...) > + cfiles = append(cfiles, a.p.CPPFiles...) > sfiles = append(sfiles, a.p.SFiles...) > > // Run cgo. > if len(a.p.CgoFiles) > 0 { > - // In a package using cgo, cgo compiles the C and assembly > files with gcc. > + // In a package using cgo, cgo compiles the C, C++ and > assembly files with gcc. > // There is one exception: runtime/cgo's job is to bridge > the > // cgo and non-cgo worlds, so it necessarily has files in > both. > // In that case gcc only gets the gcc_* files. > @@ -871,7 +872,7 @@ > } > > for _, file := range cfiles { > - out := file[:len(file)-len(".c")] + "." + objExt > + out := file[:strings.LastIndex(file, ".")] + "." + objExt > if err := buildToolchain.cc(b, a.p, obj, obj+out, file); > err != nil { > return err > } > @@ -1434,7 +1435,7 @@ > // so that it can give good error messages about forward > declarations. > // Exceptions: a few standard packages have forward declarations > for > // pieces supplied behind-the-scenes by package runtime. > - extFiles := len(p.CgoFiles) + len(p.CFiles) + len(p.SFiles) + > len(p.SysoFiles) + len(p.SwigFiles) + len(p.SwigCXXFiles) > + extFiles := len(p.CgoFiles) + len(p.CFiles) + len(p.CPPFiles) + > len(p.SFiles) + len(p.SysoFiles) + len(p.SwigFiles) + len(p.SwigCXXFiles) > if p.Standard { > switch p.ImportPath { > case "os", "runtime/pprof", "sync", "time": > Index: src/cmd/go/doc.go > ==============================**==============================**======= > --- a/src/cmd/go/doc.go > +++ b/src/cmd/go/doc.go > @@ -318,6 +318,7 @@ > CgoFiles []string // .go sources files that import "C" > IgnoredGoFiles []string // .go sources ignored due to build > constraints > CFiles []string // .c source files > + CPPFiles []string // .cc or .cpp source files > HFiles []string // .h source files > SFiles []string // .s source files > SysoFiles []string // .syso object files to add to archive > Index: src/cmd/go/list.go > ==============================**==============================**======= > --- a/src/cmd/go/list.go > +++ b/src/cmd/go/list.go > @@ -46,6 +46,7 @@ > CgoFiles []string // .go sources files that import "C" > IgnoredGoFiles []string // .go sources ignored due to build > constraints > CFiles []string // .c source files > + CPPFiles []string // .cc or .cpp source files > HFiles []string // .h source files > SFiles []string // .s source files > SysoFiles []string // .syso object files to add to archive > Index: src/cmd/go/pkg.go > ==============================**==============================**======= > --- a/src/cmd/go/pkg.go > +++ b/src/cmd/go/pkg.go > @@ -40,6 +40,7 @@ > CgoFiles []string `json:",omitempty"` // .go sources files > that import "C" > IgnoredGoFiles []string `json:",omitempty"` // .go sources ignored > due to build constraints > CFiles []string `json:",omitempty"` // .c source files > + CPPFiles []string `json:",omitempty"` // .cc or .cpp source > files > HFiles []string `json:",omitempty"` // .h source files > SFiles []string `json:",omitempty"` // .s source files > SysoFiles []string `json:",omitempty"` // .syso system object > files added to package > @@ -98,6 +99,7 @@ > p.CgoFiles = pp.CgoFiles > p.IgnoredGoFiles = pp.IgnoredGoFiles > p.CFiles = pp.CFiles > + p.CPPFiles = pp.CPPFiles > p.HFiles = pp.HFiles > p.SFiles = pp.SFiles > p.SysoFiles = pp.SysoFiles > @@ -389,6 +391,7 @@ > p.CgoFiles, > p.IgnoredGoFiles, > p.CFiles, > + p.CPPFiles, > p.HFiles, > p.SFiles, > p.SysoFiles, > @@ -611,7 +614,7 @@ > return false > } > > - srcs := stringList(p.GoFiles, p.CFiles, p.HFiles, p.SFiles, > p.CgoFiles, p.SysoFiles) > + srcs := stringList(p.GoFiles, p.CFiles, p.CPPFiles, p.HFiles, > p.SFiles, p.CgoFiles, p.SysoFiles) > for _, src := range srcs { > if olderThan(filepath.Join(p.Dir, src)) { > return true > Index: src/cmd/godoc/index.go > ==============================**==============================**======= > --- a/src/cmd/godoc/index.go > +++ b/src/cmd/godoc/index.go > @@ -657,6 +657,8 @@ > var whitelisted = map[string]bool{ > ".bash": true, > ".c": true, > + ".cc": true, > + ".cpp": true, > ".css": true, > ".go": true, > ".goc": true, > Index: src/pkg/go/build/build.go > ==============================**==============================**======= > --- a/src/pkg/go/build/build.go > +++ b/src/pkg/go/build/build.go > @@ -347,6 +347,7 @@ > CgoFiles []string // .go source files that import "C" > IgnoredGoFiles []string // .go source files ignored for this build > CFiles []string // .c source files > + CPPFiles []string // .cc or .cpp source files > HFiles []string // .h source files > SFiles []string // .s source files > SysoFiles []string // .syso system object files to add to > archive > @@ -594,7 +595,7 @@ > } > > switch ext { > - case ".go", ".c", ".s", ".h", ".S", ".swig", ".swigcxx": > + case ".go", ".c", ".cc", ".cpp", ".s", ".h", ".S", > ".swig", ".swigcxx": > // tentatively okay - read to make sure > case ".syso": > // binary objects to add to package archive > @@ -637,6 +638,9 @@ > case ".c": > p.CFiles = append(p.CFiles, name) > continue > + case ".cc", ".cpp": > + p.CPPFiles = append(p.CPPFiles, name) > + continue > case ".h": > p.HFiles = append(p.HFiles, name) > continue > > > -- > > ---You received this message because you are subscribed to the Google > Groups "golang-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou... > . > For more options, visit https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o... > . > > >
Sign in to reply to this message.
On 2013/04/02 02:40:18, bradfitz wrote: > Sorry, too late for Go 1.1. > > Please bring this up again once Go 1.1 is out and we've branched. Hi Brad, Understood. When is 1.1 supposed to be released? (so I can set a reminder, otherwise I might forget about this patch :-)).
Sign in to reply to this message.
Set a reminder to poll https://code.google.com/p/go/downloads/list in two weeks. If go1.1 isn't there, repeat. :-) On Mon, Apr 1, 2013 at 8:00 PM, <alberto.garcia.hierro@gmail.com> wrote: > On 2013/04/02 02:40:18, bradfitz wrote: > >> Sorry, too late for Go 1.1. >> > > Please bring this up again once Go 1.1 is out and we've branched. >> > > Hi Brad, > > Understood. When is 1.1 supposed to be released? (so I can set a > reminder, otherwise I might forget about this patch :-)). > > > https://codereview.appspot.**com/8248043/<https://codereview.appspot.com/8248... >
Sign in to reply to this message.
I'm pretty sure it used to not work so simply in the past, the linker improvements probably played a role. Now it seems to work with both "-linkmode external" and "-linkmode internal".
Sign in to reply to this message.
On 2013/04/02 03:20:30, bradfitz wrote: > Set a reminder to poll https://code.google.com/p/go/downloads/list in two > weeks. If go1.1 isn't there, repeat. :-) I was under the impression that it would be a matter of days, rather than weeks. If we're at least two weeks from release, I think there's enough time to finish the missing pieces of this patchset (support CXXFLAGS and call g++ rather than gcc) and update the documentation. Keep in mind that, as Remy pointed out, most of the code for supporting linking C++ is already in Go, so this patchset would only add the remaining ~10% without modifying any existing functionalities, only adding new ones (the possibility of introducing bugs to working code is almost negligible). Furthermore, I think this could be interesting to have in Go 1.1 because it's an important missing feature right now. There are a lot of C++ libraries with functionality not available in Go (in fact, I added support for C++ because I wanted to manipulate PDF files from Go and libpoppler seemed to be the only option), and currently there's no way to interact with them. Once basic support for linking CPP files is merged, we could also start working on full C++ support (e.g. having a CPP pseudo package which enables calls to functions with C++ linkage, like the C pseudo package does for C). With that said, would you reconsider your decision if I submit the missing pieces and the updated documentation within ~24 hours? Regards, Alberto
Sign in to reply to this message.
On Tue, Apr 2, 2013 at 9:44 PM, <alberto.garcia.hierro@gmail.com> wrote: > I was under the impression that it would be a matter of days, rather > than weeks. If we're at least two weeks from release, I think there's > enough time to finish the missing pieces of this patchset (support > CXXFLAGS and call g++ rather than gcc) and update the documentation. > Keep in mind that, as Remy pointed out, most of the code for supporting > linking C++ is already in Go, so this patchset would only add the > remaining ~10% without modifying any existing functionalities, only > adding new ones (the possibility of introducing bugs to working code is > almost negligible). Go 1.1 is feature frozen now. http://golang.org/s/go11freeze
Sign in to reply to this message.
message: On 2013/04/02 13:50:54, minux wrote: > Go 1.1 is feature frozen now. > > http://golang.org/s/go11freeze Ouch, I wasn't aware of that. Thanks!
Sign in to reply to this message.
minor cosmetics suggestions (CPPFiles -> CXXFiles) and add missing popular extensions (.cxx, .hxx, .hh and .C) https://codereview.appspot.com/8248043/diff/5001/src/cmd/dist/build.c File src/cmd/dist/build.c (right): https://codereview.appspot.com/8248043/diff/5001/src/cmd/dist/build.c#newcode543 src/cmd/dist/build.c:543: ".cpp", I think I would also add ".cxx", ".hh" and ".hxx" which are pretty popular extension names for C++ files. Maybe also ".C" ? https://codereview.appspot.com/8248043/diff/5001/src/cmd/dist/build.c#newcode888 src/cmd/dist/build.c:888: if(!hassuffix(files.p[i], ".c") && !hassuffix(files.p[i], ".cc") && !hassuffix(files.p[i], ".cpp") && !hassuffix(files.p[i], ".s")) add ".cxx" ? (and perhaps ".C" too ?) https://codereview.appspot.com/8248043/diff/5001/src/cmd/go/doc.go File src/cmd/go/doc.go (right): https://codereview.appspot.com/8248043/diff/5001/src/cmd/go/doc.go#newcode321 src/cmd/go/doc.go:321: CPPFiles []string // .cc or .cpp source files for symmetry with SwigCXXFiles, CPPFiles should probably be spelled out CXXFiles. I for one am always unsure when presented with CPPsomething, whether it stands for C preprocessor or C++... https://codereview.appspot.com/8248043/diff/5001/src/cmd/go/list.go File src/cmd/go/list.go (right): https://codereview.appspot.com/8248043/diff/5001/src/cmd/go/list.go#newcode49 src/cmd/go/list.go:49: CPPFiles []string // .cc or .cpp source files ditto: probably call this CXXFiles instead of CPPFiles. https://codereview.appspot.com/8248043/diff/5001/src/cmd/godoc/index.go File src/cmd/godoc/index.go (right): https://codereview.appspot.com/8248043/diff/5001/src/cmd/godoc/index.go#newco... src/cmd/godoc/index.go:661: ".cpp": true, ditto: add .cxx, .hh, .hxx and perhaps .C too. https://codereview.appspot.com/8248043/diff/5001/src/pkg/go/build/build.go File src/pkg/go/build/build.go (right): https://codereview.appspot.com/8248043/diff/5001/src/pkg/go/build/build.go#ne... src/pkg/go/build/build.go:598: case ".go", ".c", ".cc", ".cpp", ".s", ".h", ".S", ".swig", ".swigcxx": ditto: .cxx and perhaps .C too https://codereview.appspot.com/8248043/diff/5001/src/pkg/go/build/build.go#ne... src/pkg/go/build/build.go:641: case ".cc", ".cpp": ditto: .cxx and perhaps .C too.
Sign in to reply to this message.
I've implemented binet's suggestions (except for the .C extension, since it could cause problems in OSes with case insensitive filesystems) and added support for calling g++ rather than gcc for C++ files. This last patchset also adds support for CPPFLAGS (used for both C and C++) and CXXFLAGS (used only for C++). As a consequence, the pkg-config cgo directive now modifies CPPFLAGS rather than CFLAGS.
Sign in to reply to this message.
On 2013/04/07 12:26:10, Hierro wrote: > I've implemented binet's suggestions (except for the .C extension, since it > could cause problems in OSes with case insensitive filesystems) and added > support for calling g++ rather than gcc for C++ files. This last patchset also > adds support for CPPFLAGS (used for both C and C++) and CXXFLAGS (used only for > C++). As a consequence, the pkg-config cgo directive now modifies CPPFLAGS > rather than CFLAGS. ah, yes. good call on the .C extension. sebastien.
Sign in to reply to this message.
hi, just one last minor comment: the new CgoCPPFLAGS and CgoCXXFLAGS fields are missing from the command line doc. https://codereview.appspot.com/8248043/diff/21001/src/cmd/go/list.go File src/cmd/go/list.go (right): https://codereview.appspot.com/8248043/diff/21001/src/cmd/go/list.go#newcode57 src/cmd/go/list.go:57: CgoCFLAGS []string // cgo: flags for C compiler the new CgoCPPFLAGS and CgoCXXFLAGS members are missing
Sign in to reply to this message.
Hi Sebastien, Nice catch :-). I've just updated the patchset and also added a few files that I forgot to 'hg file' in the previous upload. PTAL. Regards, Alberto
Sign in to reply to this message.
I've just updated this CL with the current tip, since there were some commits affecting these files. Regards, Alberto
Sign in to reply to this message.
Now that Go 1.1 is out, it would be nice if we could discuss this CL.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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: > mailto:golang-dev@googlegroups.com), > > Please take another look. LGTM
Sign in to reply to this message.
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#newcod... misc/dist/bindist.go:859: "Mingw g++; http://sourceforge.net/projects/mingw/files/Installer/mingw-get-inst/", I don't understand why it matters that this tool, which is used to build Go distributions, be able to find g++ somewhere. https://codereview.appspot.com/8248043/diff/52001/src/cmd/dist/build.c File src/cmd/dist/build.c (right): https://codereview.appspot.com/8248043/diff/52001/src/cmd/dist/build.c#newcod... src/cmd/dist/build.c:543: ".cpp", Why do we need this? This command is used to build the Go distribution itself. We don't expect to find C++ files in the Go distribution. https://codereview.appspot.com/8248043/diff/52001/src/cmd/go/build.go File src/cmd/go/build.go (right): https://codereview.appspot.com/8248043/diff/52001/src/cmd/go/build.go#newcode188 src/cmd/go/build.go:188: // fileExtSplit expects a filename without slashes This comment needs tweaking. This will do the right thing if the filename has slashes. On the other hand, it will fail if filename has no extension. That is probably OK but the comment should be fixed. https://codereview.appspot.com/8248043/diff/52001/src/cmd/go/build.go#newcode... src/cmd/go/build.go:1706: func (b *builder) gplusplus(p *Package, out string, flags []string, cplusplusfile string) error { Let's call this method gxx. Call the last parameter cxxfile. https://codereview.appspot.com/8248043/diff/52001/src/cmd/go/build.go#newcode... src/cmd/go/build.go:1733: func (b *builder) gplusplusCmd(objdir string) []string { Call this gxxCmd. https://codereview.appspot.com/8248043/diff/52001/src/cmd/go/build.go#newcode... src/cmd/go/build.go:1925: cgocflags := stringList(cgoCPPFLAGS, cgoCFLAGS) Let's not use both cgocflags and cgoCFLAGS, that just seems confusing. Name cgocflags something else. https://codereview.appspot.com/8248043/diff/52001/src/cmd/go/build.go#newcode... src/cmd/go/build.go:1946: cgocxxflags := stringList(cgoCPPFLAGS, cgoCXXFLAGS) Similarly rename cgocxxflags. Maybe just use cflags for both. https://codereview.appspot.com/8248043/diff/52001/src/cmd/go/build.go#newcode... src/cmd/go/build.go:1949: ofile := obj + cgoRe.ReplaceAllString(file, "_") + ".o" Is this going to produce "file..o"? That doesn't seem like a good idea. Either uniquify the names in some simple way or fail. https://codereview.appspot.com/8248043/diff/52001/src/pkg/go/build/build.go File src/pkg/go/build/build.go (right): https://codereview.appspot.com/8248043/diff/52001/src/pkg/go/build/build.go#n... src/pkg/go/build/build.go:860: // These lines set CPP, CFLAGS, CXXFLAGS and LDFLAGS and pkg-config directives s/CPP/CPPFLAGS/ https://codereview.appspot.com/8248043/diff/52001/src/pkg/go/build/build.go#n... src/pkg/go/build/build.go:919: case "CPPFLAGS": These need to be documented in cmd/cgo/doc.go.
Sign in to reply to this message.
Hi Ian, Thanks for the review, I've added some comments below. On 2013/05/22 20:56:21, iant wrote: > 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#newcod... > misc/dist/bindist.go:859: "Mingw g++; > http://sourceforge.net/projects/mingw/files/Installer/mingw-get-inst/%22, > I don't understand why it matters that this tool, which is used to build Go > distributions, be able to find g++ somewhere. > > https://codereview.appspot.com/8248043/diff/52001/src/cmd/dist/build.c > File src/cmd/dist/build.c (right): > > https://codereview.appspot.com/8248043/diff/52001/src/cmd/dist/build.c#newcod... > src/cmd/dist/build.c:543: ".cpp", > Why do we need this? This command is used to build the Go distribution itself. > We don't expect to find C++ files in the Go distribution. Since g++ is usually installed along gcc, I thought allowing C++ to be used in the Go standard library wouldn't hurt. Also, I didn't knew that C++ files weren't expected in the Go distribution once support for them was added. I'll take out that code. > > https://codereview.appspot.com/8248043/diff/52001/src/cmd/go/build.go > File src/cmd/go/build.go (right): > > https://codereview.appspot.com/8248043/diff/52001/src/cmd/go/build.go#newcode188 > src/cmd/go/build.go:188: // fileExtSplit expects a filename without slashes > This comment needs tweaking. This will do the right thing if the filename has > slashes. On the other hand, it will fail if filename has no extension. That is > probably OK but the comment should be fixed. > > https://codereview.appspot.com/8248043/diff/52001/src/cmd/go/build.go#newcode... > src/cmd/go/build.go:1706: func (b *builder) gplusplus(p *Package, out string, > flags []string, cplusplusfile string) error { > Let's call this method gxx. Call the last parameter cxxfile. > > https://codereview.appspot.com/8248043/diff/52001/src/cmd/go/build.go#newcode... > src/cmd/go/build.go:1733: func (b *builder) gplusplusCmd(objdir string) []string > { > Call this gxxCmd. > > https://codereview.appspot.com/8248043/diff/52001/src/cmd/go/build.go#newcode... > src/cmd/go/build.go:1925: cgocflags := stringList(cgoCPPFLAGS, cgoCFLAGS) > Let's not use both cgocflags and cgoCFLAGS, that just seems confusing. Name > cgocflags something else. > > https://codereview.appspot.com/8248043/diff/52001/src/cmd/go/build.go#newcode... > src/cmd/go/build.go:1946: cgocxxflags := stringList(cgoCPPFLAGS, cgoCXXFLAGS) > Similarly rename cgocxxflags. Maybe just use cflags for both. Noted all name changes. > > https://codereview.appspot.com/8248043/diff/52001/src/cmd/go/build.go#newcode... > src/cmd/go/build.go:1949: ofile := obj + cgoRe.ReplaceAllString(file, "_") + > ".o" > Is this going to produce "file..o"? That doesn't seem like a good idea. Either > uniquify the names in some simple way or fail. That's a bug. The idea was producing file.c.o from file.c and file.cpp.o from file.cpp. This way a package can have both file.c and file.cpp and build without issues. I'll fix that. > > https://codereview.appspot.com/8248043/diff/52001/src/pkg/go/build/build.go > File src/pkg/go/build/build.go (right): > > https://codereview.appspot.com/8248043/diff/52001/src/pkg/go/build/build.go#n... > src/pkg/go/build/build.go:860: // These lines set CPP, CFLAGS, CXXFLAGS and > LDFLAGS and pkg-config directives > s/CPP/CPPFLAGS/ > > https://codereview.appspot.com/8248043/diff/52001/src/pkg/go/build/build.go#n... > src/pkg/go/build/build.go:919: case "CPPFLAGS": > These need to be documented in cmd/cgo/doc.go. Roger that.
Sign in to reply to this message.
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.
Sign in to reply to this message.
R=rsc (assigned by dsymonds)
Sign in to reply to this message.
On 2013/05/27 00:54:14, gobot wrote: > R=rsc (assigned by dsymonds) Ping
Sign in to reply to this message.
-rsc +iant
Sign in to reply to this message.
Please run hg mail 8248043 to refresh the codereview repository.
Sign in to reply to this message.
I'm seeing "chunk mismatch" in rietveld. Sorry, but can you upload the CL again?
Sign in to reply to this message.
On 2013/05/31 14:21:29, iant wrote: > I'm seeing "chunk mismatch" in rietveld. Sorry, but can you upload the CL > again? I Ian, I've just uploaded it (after hg sync). Regards, Alberto
Sign in to reply to this message.
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 pseudo #cgo directives what is CPPFLAGS? what is CXXFLAGS? i guess these flags are defined by gcc and g++ but it wouldn't hurt to explain that. they're confusing out of context. https://codereview.appspot.com/8248043/diff/80003/src/cmd/cgo/doc.go#newcode39 src/cmd/cgo/doc.go:39: Alternatively, CPPFLAGS and LDFLAGS may be obtained via the pkg-config where did CFLAGS go? https://codereview.appspot.com/8248043/diff/80003/src/cmd/go/build.go File src/cmd/go/build.go (right): https://codereview.appspot.com/8248043/diff/80003/src/cmd/go/build.go#newcode188 src/cmd/go/build.go:188: // fileExtSplit expects a filename and returns the name "expects"? https://codereview.appspot.com/8248043/diff/80003/src/cmd/go/build.go#newcode192 src/cmd/go/build.go:192: dotExt := path.Ext(file) you should be using path/filepath. also i think the library can help more here than you're letting it.
Sign in to reply to this message.
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#newcod... src/cmd/dist/build.c:889: if(!hassuffix(files.p[i], ".c") && !hassuffix(files.p[i], ".cc") && !hassuffix(files.p[i], ".cpp") && !hassuffix(files.p[i], ".cxx") && !hassuffix(files.p[i], ".s")) We don't need this change. Please drop it. https://codereview.appspot.com/8248043/diff/80003/src/cmd/go/build.go File src/cmd/go/build.go (right): https://codereview.appspot.com/8248043/diff/80003/src/cmd/go/build.go#newcode829 src/cmd/go/build.go:829: outGo, outObj, err := b.swig(a.p, obj, gccfiles) Please add a TODO comment saying that we should handle a.p.CXXFiles with swig (it doesn't currently handle .c files reasonably either). https://codereview.appspot.com/8248043/diff/80003/src/cmd/go/build.go#newcode836 src/cmd/go/build.go:836: If this package uses neither cgo nor swig, it looks a.p.CXXFiles is ignored. That doesn't seem right. We should probably give an error in that case. https://codereview.appspot.com/8248043/diff/80003/src/cmd/go/build.go#newcode... src/cmd/go/build.go:1962: ofile := obj + cgoRe.ReplaceAllString(file, "_") + ".o" I thought this was going to be changed. It seems like it will produce file..o.
Sign in to reply to this message.
Hi Ian, Thanks for the review. Comments are below. I've also uploaded a new patchset with these changes. Regards, Alberto On 2013/05/31 15:44:05, iant wrote: > 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#newcod... > src/cmd/dist/build.c:889: if(!hassuffix(files.p[i], ".c") && > !hassuffix(files.p[i], ".cc") && !hassuffix(files.p[i], ".cpp") && > !hassuffix(files.p[i], ".cxx") && !hassuffix(files.p[i], ".s")) > We don't need this change. Please drop it. Done > > https://codereview.appspot.com/8248043/diff/80003/src/cmd/go/build.go > File src/cmd/go/build.go (right): > > https://codereview.appspot.com/8248043/diff/80003/src/cmd/go/build.go#newcode829 > src/cmd/go/build.go:829: outGo, outObj, err := b.swig(a.p, obj, gccfiles) > Please add a TODO comment saying that we should handle a.p.CXXFiles with swig > (it doesn't currently handle .c files reasonably either). Done > > https://codereview.appspot.com/8248043/diff/80003/src/cmd/go/build.go#newcode836 > src/cmd/go/build.go:836: > If this package uses neither cgo nor swig, it looks a.p.CXXFiles is ignored. > That doesn't seem right. We should probably give an error in that case. Done > > https://codereview.appspot.com/8248043/diff/80003/src/cmd/go/build.go#newcode... > src/cmd/go/build.go:1962: ofile := obj + cgoRe.ReplaceAllString(file, "_") + > ".o" > I thought this was going to be changed. It seems like it will produce file..o. I reviewed the code and it was already working as intended. "file" variable contains the absolute path to the file, so file.cpp produces file.cpp.o. Maybe you're getting confused with the previous for loop (the one acting on .c files) which uses file[:len(file)-1]? Just ran go build -x on a package using .cpp files, to make sure: fiam@ubuntu:~/gocode/src/poppler$ go build -x ... g++ -I . -g -O2 -fPIC -m64 -pthread -I/usr/include/poppler -I $WORK/poppler/_obj/ -o $WORK/poppler/_obj/poppler.cpp.o -c ./poppler.cpp …
Sign in to reply to this message.
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 > src/cmd/cgo/doc.go:27: CPPFLAGS, CFLAGS, CXXFLAGS and LDFLAGS may be defined > with pseudo #cgo directives > what is CPPFLAGS? what is CXXFLAGS? Nowhere in that page are CFLAGS and LDFLAGS explained, and I think anyone interested in linking C code into a Go program already knows what those are (yup, including CPPFLAGS and CXXFLAGS). > > i guess these flags are defined by gcc and g++ but it wouldn't hurt to explain > that. they're confusing out of context. It wouldn't hurt, but documenting CPPFLAGS, CFLAGS, CXXFLAGS and LDFLAGS is not in the scope of this CL. CFLAGS and LDFLAGS have been mentioned in that document since before Go 1.0 and they aren't documented. > > https://codereview.appspot.com/8248043/diff/80003/src/cmd/cgo/doc.go#newcode39 > src/cmd/cgo/doc.go:39: Alternatively, CPPFLAGS and LDFLAGS may be obtained via > the pkg-config > where did CFLAGS go? CPPFLAGS are passed to both C and C++ files (it stands for C preprocessor flags), as I explained in the description of this CL, so pkg-config now sets CPPFLAGS rather than CFLAGS. > > https://codereview.appspot.com/8248043/diff/80003/src/cmd/go/build.go > File src/cmd/go/build.go (right): > > https://codereview.appspot.com/8248043/diff/80003/src/cmd/go/build.go#newcode188 > src/cmd/go/build.go:188: // fileExtSplit expects a filename and returns the name > "expects"? English is not my first nor my second language. You're welcome to s/expects/whatever/ in that comment if you think of a better verb. > > https://codereview.appspot.com/8248043/diff/80003/src/cmd/go/build.go#newcode192 > src/cmd/go/build.go:192: dotExt := path.Ext(file) > you should be using path/filepath. also i think the library can help more here > than you're letting it. Just changed path to filepath (although only files with an extension are passed to that function, so it won't make any difference). On the other hand, I can't find any function in path nor path/filepath which returns the name of a file minus the extension. I'll gladly use it if it's available (and if it's not, it would be a welcome addition).
Sign in to reply to this message.
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#newcod... src/cmd/go/build.go:748: return fmt.Errorf("Can't build package %s because it contains C++ files (%s) but it's not using cgo nor SWIG", s/Can't/can't/ Error strings start with lower case letters.
Sign in to reply to this message.
LGTM you're right - it seems there is a gap in the path and filepath APIs.
Sign in to reply to this message.
On 2013/05/31 18:16:26, iant wrote: > 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#newcod... > src/cmd/go/build.go:748: return fmt.Errorf("Can't build package %s because it > contains C++ files (%s) but it's not using cgo nor SWIG", > s/Can't/can't/ > Error strings start with lower case letters. I wasn't aware of that. Thanks! I've just updated the CL. Regards, Alberto
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=24596e5bca7d *** 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. R=iant, r CC=bradfitz, gobot, golang-dev, iant, minux.ma, remyoudompheng, seb.binet https://codereview.appspot.com/8248043 Committer: Ian Lance Taylor <iant@golang.org>
Sign in to reply to this message.
Message was sent while issue was closed.
On 2013/05/31 18:20:16, r wrote: > LGTM > > you're right - it seems there is a gap in the path and filepath APIs. Thanks! Let's try to close that gap. How does this CL look like https://codereview.appspot.com/9908043 ? Regards, Alberto
Sign in to reply to this message.
Should we update doc/go1.2.txt? On Sat, Jun 1, 2013 at 2:33 AM, <iant@golang.org> wrote: > *** Submitted as > https://code.google.com/p/go/**source/detail?r=24596e5bca7d<https://code.goog... > > 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. > > R=iant, r > CC=bradfitz, gobot, golang-dev, iant, minux.ma, remyoudompheng, > seb.binet > https://codereview.appspot.**com/8248043<https://codereview.appspot.com/8248043> > > Committer: Ian Lance Taylor <iant@golang.org> > > > https://codereview.appspot.**com/8248043/<https://codereview.appspot.com/8248... >
Sign in to reply to this message.
On Fri, May 31, 2013 at 6:54 PM, minux <minux.ma@gmail.com> wrote: > Should we update doc/go1.2.txt? Yes!
Sign in to reply to this message.
|