Issue 7331048: code review 7331048: cmd/go: pass -intgosize to SWIG (Closed)
|
Created:
11 years, 3 months ago by cookieo9 Modified:
11 years, 2 months ago Reviewers:
CC:
golang-dev, minux1, iant Visibility:
Public. |
Descriptioncmd/go: pass -intgosize to SWIG
swig >= 2.0.9 requires the size of int values to be passed via a command line flag. Should swig complain about the -intgosize not being supported, then alert the user to their outdated version of swig.
Fixes issue 4756.
Patch Set 1 #Patch Set 2 : diff -r d57e0c4793d4 http://code.google.com/p/go #Patch Set 3 : diff -r 41ab0ca75ea3 http://code.google.com/p/go #Patch Set 4 : diff -r 41ab0ca75ea3 http://code.google.com/p/go #
Total comments: 1
Patch Set 5 : diff -r 59da6744d66d http://code.google.com/p/go #
Total comments: 4
Patch Set 6 : diff -r 59da6744d66d http://code.google.com/p/go #Patch Set 7 : diff -r 2cf1ef5d96c6 http://code.google.com/p/go #
Total comments: 1
Patch Set 8 : diff -r 2cf1ef5d96c6 http://code.google.com/p/go #Patch Set 9 : diff -r 864c37196df8 http://code.google.com/p/go #
Total comments: 2
Patch Set 10 : diff -r 0add1de89fe4 http://code.google.com/p/go #
Total comments: 13
Patch Set 11 : diff -r 865d2efc4bf0 http://code.google.com/p/go #
Total comments: 2
Patch Set 12 : diff -r c3733f9d8642 http://code.google.com/p/go #MessagesTotal messages: 33
Hello golang-dev@googlegroups.com, I'd like you to review this change to http://code.google.com/p/go
Sign in to reply to this message.
r: +iant https://codereview.appspot.com/7331048/diff/7001/src/cmd/go/build.go File src/cmd/go/build.go (right): https://codereview.appspot.com/7331048/diff/7001/src/cmd/go/build.go#newcode1928 src/cmd/go/build.go:1928: if runtime.GOARCH == "amd64" { we probably should detect this by testing (u)int has 64-bit, then we can also handle 64-bit architectures supported by gccgo (when using -compiler gccgo, we probably should ask gccgo for its size of int) we also need to consider what if the user use 32-bit Go tool on 64-bit host and set GOARCH=amd64?
Sign in to reply to this message.
On Fri, Feb 15, 2013 at 10:50 AM, <minux.ma@gmail.com> wrote: > > https://codereview.appspot.com/7331048/diff/7001/src/cmd/go/build.go#newcode1928 > src/cmd/go/build.go:1928: if runtime.GOARCH == "amd64" { > we probably should detect this by testing (u)int has 64-bit, > then we can also handle 64-bit architectures supported by > gccgo (when using -compiler gccgo, we probably should > ask gccgo for its size of int) But that won't work because > we also need to consider what if the user use 32-bit Go tool > on 64-bit host and set GOARCH=amd64? Exactly. Testing GOARCH gives the right answer in principle, testing the size of uint does not. Unless somebody can think of an easy way to ask the compiler, please write the code as a switch on GOARCH, list the known cases, and fail if there is no case that matches the GOARCH value. Ian
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, minux.ma@gmail.com, iant@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
https://codereview.appspot.com/7331048/diff/14001/src/cmd/go/build.go File src/cmd/go/build.go (right): https://codereview.appspot.com/7331048/diff/14001/src/cmd/go/build.go#newcode... src/cmd/go/build.go:1934: return "", "", errors.New("SWIG is not supported on GOARCH=" + goarch) I don't think that's the right error message. I think it should be something like SWIG support unimplemented on GOARCH=%s; edit cmd/go/build.go https://codereview.appspot.com/7331048/diff/14001/src/cmd/go/build.go#newcode... src/cmd/go/build.go:1955: return "", "", errors.New("Must have SWIG version >= 2.0.9\n") Errors should start with lower case letters by convention.
Sign in to reply to this message.
On Sat, Feb 16, 2013 at 3:06 AM, Ian Lance Taylor <iant@golang.org> wrote: > Unless somebody can think of an easy way to ask the compiler, please > write the code as a switch on GOARCH, list the known cases, and fail > if there is no case that matches the GOARCH value. > perhaps we should add a flag to each compiler for this before too late. (or test compiler version?) what if the user is using Go 1.1 cmd/go with gccgo 4.7.2? this doesn't seem unreasonable, as understandably distributions lag on gcc update, but they normally ship newly released Go soon after its release. i don't think we should reject the use of gccgo 4.7.2 either.
Sign in to reply to this message.
On Fri, Feb 15, 2013 at 11:39 AM, minux <minux.ma@gmail.com> wrote: > > On Sat, Feb 16, 2013 at 3:06 AM, Ian Lance Taylor <iant@golang.org> wrote: >> >> Unless somebody can think of an easy way to ask the compiler, please >> write the code as a switch on GOARCH, list the known cases, and fail >> if there is no case that matches the GOARCH value. > > perhaps we should add a flag to each compiler for this before too late. > (or test compiler version?) > > what if the user is using Go 1.1 cmd/go with gccgo 4.7.2? > this doesn't seem unreasonable, as understandably distributions lag > on gcc update, but they normally ship newly released Go soon after > its release. > > i don't think we should reject the use of gccgo 4.7.2 either. I'm willing to go in that direction if somebody wants to do the work. I agree that it is generally better. Ian
Sign in to reply to this message.
https://codereview.appspot.com/7331048/diff/14001/src/cmd/go/build.go File src/cmd/go/build.go (right): https://codereview.appspot.com/7331048/diff/14001/src/cmd/go/build.go#newcode... src/cmd/go/build.go:1934: return "", "", errors.New("SWIG is not supported on GOARCH=" + goarch) On 2013/02/15 19:39:25, iant wrote: > I don't think that's the right error message. I think it should be something > like > SWIG support unimplemented on GOARCH=%s; edit cmd/go/build.go Done. Should I use fmt.Errorf here or is string concatenation OK? https://codereview.appspot.com/7331048/diff/14001/src/cmd/go/build.go#newcode... src/cmd/go/build.go:1955: return "", "", errors.New("Must have SWIG version >= 2.0.9\n") On 2013/02/15 19:39:25, iant wrote: > Errors should start with lower case letters by convention. Done.
Sign in to reply to this message.
On Fri, Feb 15, 2013 at 11:46 AM, <cookieo9@gmail.com> wrote: >> SWIG support unimplemented on GOARCH=%s; edit cmd/go/build.go > > > Done. Should I use fmt.Errorf here or is string concatenation OK? String concatenation is fine. Ian
Sign in to reply to this message.
On 2013/02/15 19:42:08, iant wrote: > On Fri, Feb 15, 2013 at 11:39 AM, minux <mailto:minux.ma@gmail.com> wrote: > > > > On Sat, Feb 16, 2013 at 3:06 AM, Ian Lance Taylor <mailto:iant@golang.org> wrote: > >> > >> Unless somebody can think of an easy way to ask the compiler, please > >> write the code as a switch on GOARCH, list the known cases, and fail > >> if there is no case that matches the GOARCH value. > > > > perhaps we should add a flag to each compiler for this before too late. > > (or test compiler version?) > > > > what if the user is using Go 1.1 cmd/go with gccgo 4.7.2? > > this doesn't seem unreasonable, as understandably distributions lag > > on gcc update, but they normally ship newly released Go soon after > > its release. > > > > i don't think we should reject the use of gccgo 4.7.2 either. > > I'm willing to go in that direction if somebody wants to do the work. > > I agree that it is generally better. > > Ian Am I waiting then on future changes to gc/gccgo? Or will this go through and be patched later to support those theoretical flags?
Sign in to reply to this message.
On Sat, Feb 16, 2013 at 3:55 AM, <cookieo9@gmail.com> wrote: > Am I waiting then on future changes to gc/gccgo? Or will this go through > and be patched later to support those theoretical flags? we have several alternatives for gccgo support: 1. only support 32-bit pre-Go 1.1 gccgo: this support is the bottom line, IMO, because int is still 32-bit with those. 2. also support 64-bit pre-Go 1.1 gccgo, and pass -intgosize 32 to swig in this case. 3. we pass compiler to swig, and let swig figure out the correct -intgosize, then we require that version of swig unconditionally for Go 1.1. Ian, is it still possible to add the required flag to gcc 4.8 (we cannot let any Go 1.1 gccgo release or the whole scheme fails). Once we've selected a way to go, we can decide whether to commit this CL now or later.
Sign in to reply to this message.
On Fri, Feb 15, 2013 at 12:09 PM, minux <minux.ma@gmail.com> wrote: > > Ian, is it still possible to add the required flag to gcc 4.8 (we cannot let > any > Go 1.1 gccgo release or the whole scheme fails). What do you think about doing something like compile this file: package p import "unsafe" const IntSize = unsafe.Sizeof(int(0)) and grep the object file for IntSize? It will show up in the export information. Ian
Sign in to reply to this message.
On Sat, Feb 16, 2013 at 5:31 AM, Ian Lance Taylor <iant@golang.org> wrote: > On Fri, Feb 15, 2013 at 12:09 PM, minux <minux.ma@gmail.com> wrote: > > > > Ian, is it still possible to add the required flag to gcc 4.8 (we cannot > let > > any > > Go 1.1 gccgo release or the whole scheme fails). > > What do you think about doing something like compile this file: > > package p > > import "unsafe" > > const IntSize = unsafe.Sizeof(int(0)) > > and grep the object file for IntSize? It will show up in the export > information. > how about just trying to compile this program (only necessary for 64-bit arch)? package p const i int = 1<<32 should be easier than grepping object file. what's your opinion about supporting 64-bit pre-Go 1.1 gccgo with Go 1.1 cmd/go? if we all agree, maybe we can implement this scheme in this CL?
Sign in to reply to this message.
On Fri, Feb 15, 2013 at 1:42 PM, minux <minux.ma@gmail.com> wrote: > > how about just trying to compile this program (only necessary for 64-bit > arch)? > > package p > const i int = 1<<32 Good, even better. Thanks. > what's your opinion about supporting 64-bit pre-Go 1.1 gccgo with Go 1.1 > cmd/go? I don't care very much, but I agree it would be better to have support than not. Ian
Sign in to reply to this message.
On 2013/02/15 21:46:40, iant wrote: > On Fri, Feb 15, 2013 at 1:42 PM, minux <mailto:minux.ma@gmail.com> wrote: > > > > how about just trying to compile this program (only necessary for 64-bit > > arch)? > > > > package p > > const i int = 1<<32 > > Good, even better. Thanks. > > > > what's your opinion about supporting 64-bit pre-Go 1.1 gccgo with Go 1.1 > > cmd/go? > > I don't care very much, but I agree it would be better to have support than > not. > > Ian I have no problem with the concept of doing this (building a sample go file), but I am not so familiar with the proper mechanisms to do this. I would obviously like to do this with the facilities in the cmd/go codebase, any suggestions on how before I jump in?
Sign in to reply to this message.
On Fri, Feb 15, 2013 at 1:58 PM, <cookieo9@gmail.com> wrote: > > I have no problem with the concept of doing this (building a sample go > file), but I am not so familiar with the proper mechanisms to do this. I > would obviously like to do this with the facilities in the cmd/go > codebase, any suggestions on how before I jump in? Create a temporary file in the b.work directory. Use the gc method of buildToolchain. Ian
Sign in to reply to this message.
On 2013/02/15 22:12:46, iant wrote: > On Fri, Feb 15, 2013 at 1:58 PM, <mailto:cookieo9@gmail.com> wrote: > > > > I have no problem with the concept of doing this (building a sample go > > file), but I am not so familiar with the proper mechanisms to do this. I > > would obviously like to do this with the facilities in the cmd/go > > codebase, any suggestions on how before I jump in? > > Create a temporary file in the b.work directory. Use the gc method of > buildToolchain. > > Ian Done. PTAL. One thing, I can't suppress the output of buildToolchain.gc without changing it and the toolchain interface, so currently on 32-bit systems, an (ignored) error is printed out in the go tool output. I shied away from modifying such a critical piece of cmd/go, until at least my other logic was considered acceptable. I can only test 8g and 6g since AFAIK gccgo doesn't work on Mac OS X.
Sign in to reply to this message.
This looks good to me once we fix that error message. Thanks for your patience with this. https://codereview.appspot.com/7331048/diff/12002/src/cmd/go/build.go File src/cmd/go/build.go (right): https://codereview.appspot.com/7331048/diff/12002/src/cmd/go/build.go#newcode... src/cmd/go/build.go:1929: if _, err = buildToolchain.gc(b, p, obj, nil, srcs); err != nil { I suppose you need a version of gc that calls runOut rather than run.
Sign in to reply to this message.
On 2013/02/15 23:39:26, iant wrote: > This looks good to me once we fix that error message. Thanks for your patience > with this. > > https://codereview.appspot.com/7331048/diff/12002/src/cmd/go/build.go > File src/cmd/go/build.go (right): > > https://codereview.appspot.com/7331048/diff/12002/src/cmd/go/build.go#newcode... > src/cmd/go/build.go:1929: if _, err = buildToolchain.gc(b, p, obj, nil, srcs); > err != nil { > I suppose you need a version of gc that calls runOut rather than run. I solved the problem by replacing builder.print for the duration of the buildToolchain.gc call. Is it a little too hack-ish?
Sign in to reply to this message.
On 2013/02/16 00:16:34, cookieo9 wrote: > > I solved the problem by replacing builder.print for the duration of the > buildToolchain.gc call. Is it a little too hack-ish? That sounds a little too hackish to me. How about changing the gc method to return the output as runOut does. I only see one call to the gc method anyhow.
Sign in to reply to this message.
On 2013/02/16 00:42:32, iant wrote: > On 2013/02/16 00:16:34, cookieo9 wrote: > > > > I solved the problem by replacing builder.print for the duration of the > > buildToolchain.gc call. Is it a little too hack-ish? > > That sounds a little too hackish to me. How about changing the gc method to > return the output as runOut does. I only see one call to the gc method anyhow. Could I pass a suppress-output flag to gc(), so that I don't have to re-write most of run() in gc().
Sign in to reply to this message.
> Could I pass a suppress-output flag to gc(), so that I don't have to re-write > most of run() in gc(). I have uploaded what a suppress output solution might look like. PTAL.
Sign in to reply to this message.
Sorry for letting this sit. https://codereview.appspot.com/7331048/diff/14002/src/cmd/go/build.go File src/cmd/go/build.go (right): https://codereview.appspot.com/7331048/diff/14002/src/cmd/go/build.go#newcode... src/cmd/go/build.go:1511: if suppressOutput { This repeated code just looks strange to me. Why not have gc call runOut and return the output. The code in run that massages the output can be broken out into a function that is called by the current single caller of gc.
Sign in to reply to this message.
https://codereview.appspot.com/7331048/diff/14002/src/cmd/go/build.go File src/cmd/go/build.go (right): https://codereview.appspot.com/7331048/diff/14002/src/cmd/go/build.go#newcode... src/cmd/go/build.go:1511: if suppressOutput { On 2013/02/27 01:47:57, iant wrote: > This repeated code just looks strange to me. Why not have gc call runOut and > return the output. The code in run that massages the output can be broken out > into a function that is called by the current single caller of gc. Done, I'm not sure how much I'm saving, because the code which processes the output needs a bunch of extra logic to be in place where gc is used. Also many of the variables that code uses are cooked a little in run(), which makes the code at the call-site of gc a little uglier. On the upside I can use the new output processing code to fix up the changes I made.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, minux.ma@gmail.com, iant@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
https://codereview.appspot.com/7331048/diff/35001/src/cmd/go/build.go File src/cmd/go/build.go (right): https://codereview.appspot.com/7331048/diff/35001/src/cmd/go/build.go#newcode... src/cmd/go/build.go:1205: func (b *builder) generateDescription(dir string, cmdargs ...interface{}) string { I created the generateDescription to generate the desc argument to b.showOutput, but it's annoying to use since it uses cmdargs which is generated by gc() which makes it difficult to use outside gc(). The logic is only meant to provide a default should desc be the empty string, so hopefully, the code which calls gc, where desc is the package name will never be "". I will probably remove this function if I can't find a good way to use it as it currently is a waste of space (adds 3 lines for no benefit). I will have to put it back in run() then. https://codereview.appspot.com/7331048/diff/35001/src/cmd/go/build.go#newcode... src/cmd/go/build.go:1209: func (b *builder) processOutput(out []byte) string { Any suggestions on name? Should I put a method comment? https://codereview.appspot.com/7331048/diff/35001/src/cmd/go/build.go#newcode... src/cmd/go/build.go:1227: func (b *builder) runOut(dir string, desc string, cmdargs ...interface{}) ([]byte, error) { I noticed the desc arg in this function is never used, should I do anything about it?
Sign in to reply to this message.
Thanks for your patience on this. https://codereview.appspot.com/7331048/diff/35001/src/cmd/go/build.go File src/cmd/go/build.go (right): https://codereview.appspot.com/7331048/diff/35001/src/cmd/go/build.go#newcode821 src/cmd/go/build.go:821: b.showOutput(a.p.Dir, desc, messages) Just write b.showOutput(a.p.Dir, a.p.ImportPath, b.processOutput(out)) https://codereview.appspot.com/7331048/diff/35001/src/cmd/go/build.go#newcode... src/cmd/go/build.go:1197: b.showOutput(dir, desc, messages) b.showOutput(dir, desc, b.processOutput(out)) https://codereview.appspot.com/7331048/diff/35001/src/cmd/go/build.go#newcode... src/cmd/go/build.go:1205: func (b *builder) generateDescription(dir string, cmdargs ...interface{}) string { On 2013/02/27 04:25:22, cookieo9 wrote: > I created the generateDescription to generate the desc argument to b.showOutput, > but it's annoying to use since it uses cmdargs which is generated by gc() which > makes it difficult to use outside gc(). > > The logic is only meant to provide a default should desc be the empty string, > so hopefully, the code which calls gc, where desc is the package name will never > be "". > > I will probably remove this function if I can't find a good way to use it as it > currently is a waste of space (adds 3 lines for no benefit). > > I will have to put it back in run() then. At the moment you don't seem to be using generateDescription at all. Did I miss something? The lines in run to handle desc == "" don't seem to exist any more here. https://codereview.appspot.com/7331048/diff/35001/src/cmd/go/build.go#newcode... src/cmd/go/build.go:1209: func (b *builder) processOutput(out []byte) string { On 2013/02/27 04:25:22, cookieo9 wrote: > Any suggestions on name? Should I put a method comment? I think the name is fine. Yes, it should have a method comment, as should generateDescription. https://codereview.appspot.com/7331048/diff/35001/src/cmd/go/build.go#newcode... src/cmd/go/build.go:1227: func (b *builder) runOut(dir string, desc string, cmdargs ...interface{}) ([]byte, error) { On 2013/02/27 04:25:22, cookieo9 wrote: > I noticed the desc arg in this function is never used, should I do anything > about it? Let's leave that for a different CL, if we care.
Sign in to reply to this message.
https://codereview.appspot.com/7331048/diff/35001/src/cmd/go/build.go File src/cmd/go/build.go (right): https://codereview.appspot.com/7331048/diff/35001/src/cmd/go/build.go#newcode821 src/cmd/go/build.go:821: b.showOutput(a.p.Dir, desc, messages) On 2013/02/27 14:43:45, iant wrote: > Just write > b.showOutput(a.p.Dir, a.p.ImportPath, b.processOutput(out)) Done, I also removed the extra "messages" variable, similar to as you suggested for below. https://codereview.appspot.com/7331048/diff/35001/src/cmd/go/build.go#newcode... src/cmd/go/build.go:1197: b.showOutput(dir, desc, messages) On 2013/02/27 14:43:45, iant wrote: > b.showOutput(dir, desc, b.processOutput(out)) Done. https://codereview.appspot.com/7331048/diff/35001/src/cmd/go/build.go#newcode... src/cmd/go/build.go:1205: func (b *builder) generateDescription(dir string, cmdargs ...interface{}) string { On 2013/02/27 14:43:45, iant wrote: > On 2013/02/27 04:25:22, cookieo9 wrote: > > I created the generateDescription to generate the desc argument to > b.showOutput, > > but it's annoying to use since it uses cmdargs which is generated by gc() > which > > makes it difficult to use outside gc(). > > > > The logic is only meant to provide a default should desc be the empty string, > > so hopefully, the code which calls gc, where desc is the package name will > never > > be "". > > > > I will probably remove this function if I can't find a good way to use it as > it > > currently is a waste of space (adds 3 lines for no benefit). > > > > I will have to put it back in run() then. > > At the moment you don't seem to be using generateDescription at all. Did I miss > something? The lines in run to handle desc == "" don't seem to exist any more > here. As it turns out I never did use it, my intention when I wrote the function was to have the callers of gc do it, but then I realized the problem (a lack of command ilne arguments there). I also realized this would change the behaviour for other calls to run, so in my next patch I will put this back in run() and hope that all calls to gc will always have a package set. https://codereview.appspot.com/7331048/diff/35001/src/cmd/go/build.go#newcode... src/cmd/go/build.go:1209: func (b *builder) processOutput(out []byte) string { On 2013/02/27 14:43:45, iant wrote: > On 2013/02/27 04:25:22, cookieo9 wrote: > > Any suggestions on name? Should I put a method comment? > > I think the name is fine. Yes, it should have a method comment, as should > generateDescription. Done. https://codereview.appspot.com/7331048/diff/35001/src/cmd/go/build.go#newcode... src/cmd/go/build.go:1227: func (b *builder) runOut(dir string, desc string, cmdargs ...interface{}) ([]byte, error) { On 2013/02/27 14:43:45, iant wrote: > On 2013/02/27 04:25:22, cookieo9 wrote: > > I noticed the desc arg in this function is never used, should I do anything > > about it? > > Let's leave that for a different CL, if we care. Ok, aside from here, there would be 4+ call sites that would need to be modified.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, minux.ma@gmail.com, iant@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
https://codereview.appspot.com/7331048/diff/42001/src/cmd/go/build.go File src/cmd/go/build.go (right): https://codereview.appspot.com/7331048/diff/42001/src/cmd/go/build.go#newcode... src/cmd/go/build.go:1953: func (b *builder) swigIntSize(obj string) (intsize string, err error) { Should I comment this new function as well?
Sign in to reply to this message.
https://codereview.appspot.com/7331048/diff/42001/src/cmd/go/build.go File src/cmd/go/build.go (right): https://codereview.appspot.com/7331048/diff/42001/src/cmd/go/build.go#newcode... src/cmd/go/build.go:1953: func (b *builder) swigIntSize(obj string) (intsize string, err error) { On 2013/02/27 16:04:23, cookieo9 wrote: > Should I comment this new function as well? I have done so since the answer will probably be yes, and if it's no it's easy enough to fix.
Sign in to reply to this message.
LGTM Thanks for bearing with me.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=3d1a771ddcc5 *** cmd/go: pass -intgosize to SWIG swig >= 2.0.9 requires the size of int values to be passed via a command line flag. Should swig complain about the -intgosize not being supported, then alert the user to their outdated version of swig. Fixes issue 4756. R=golang-dev, minux.ma, iant CC=golang-dev https://codereview.appspot.com/7331048 Committer: Ian Lance Taylor <iant@golang.org>
Sign in to reply to this message.
|
