code review 5700044: dist: treat CC as one unit

dist: treat CC as one unit

Fixes issue 3112.

[bytbox, 2012-02-23 09:01:28]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[atom, 2012-02-23 10:42:55]

On 2012/02/23 09:01:28, bytbox wrote:
> Hello mailto:golang-dev@googlegroups.com (cc:
mailto:golang-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://go.googlecode.com/hg/

In my opinion, the patch can be improved in the following way (conceptually):

  static Vec gccargs;
  static int gccargs_cmdlen;
  ...
  xgetenv(&b, "CC");
  gccargs_cmdlen = <the number of fields in CC>;
  ...
  vcopy(&link, gccargs.p, gccargs_cmdlen);

[atom, 2012-02-23 10:45:05]

On 2012/02/23 10:42:55, atom wrote:
> On 2012/02/23 09:01:28, bytbox wrote:
> > Hello mailto:golang-dev@googlegroups.com (cc:
> mailto:golang-dev@googlegroups.com),
> > 
> > I'd like you to review this change to
> > https://go.googlecode.com/hg/
> 
> In my opinion, the patch can be improved in the following way (conceptually):
> 
>   static Vec gccargs;
>   static int gccargs_cmdlen;
>   ...
>   xgetenv(&b, "CC");
>   gccargs_cmdlen = <the number of fields in CC>;
>   ...
>   vcopy(&link, gccargs.p, gccargs_cmdlen);

Also, the following line:

  if(xstrstr(gccargs.p[0], "clang") != nil) {

should be changed into:

  if(xstrstr(gccargs.p[gccargs_cmdlen-1], "clang") != nil) {

[ality, 2012-02-23 11:47:03]

bytbox@gmail.com once said:
>  		// C command.
>  		// Use gccargs, but ensure that link.p[2] is output file,
>  		// as noted above.
> -		vadd(&link, gccargs.p[0]);
> +		vcopy(&link, gccargs.p, gccargs.len-1);
>  		vadd(&link, "-o");
>  		vadd(&link, bpathf(&b, "%s/%s%s", tooldir, name, exe));
> -		vcopy(&link, gccargs.p+1, gccargs.len-1);
>  		if(streq(gohostarch, "amd64"))
>  			vadd(&link, "-m64");
>  		else if(streq(gohostarch, "386"))

Doesn't this change go against the comment directly above it?
Now link.p[2] won't be the output file.

  Anthony

[bytbox, 2012-02-23 16:00:46]

Hello golang-dev@googlegroups.com, 0xe2.0x9a.0x9b@gmail.com, ality@pbrane.org
(cc: golang-dev@googlegroups.com),

Please take another look.

[rsc1, 2012-02-23 16:31:16]

http://codereview.appspot.com/5700044/diff/6005/src/cmd/dist/build.c
File src/cmd/dist/build.c (right):

http://codereview.appspot.com/5700044/diff/6005/src/cmd/dist/build.c#newcode542
src/cmd/dist/build.c:542: int i, j, k, n, doclean, targi;
s/targi/targ/

http://codereview.appspot.com/5700044/diff/6005/src/cmd/dist/build.c#newcode613
src/cmd/dist/build.c:613: targi = 2;
Please move back 1 line and use targi = link.len;
Same for the others.

http://codereview.appspot.com/5700044/diff/6005/src/cmd/dist/build.c#newcode634
src/cmd/dist/build.c:634: vcopy(&link, gccargs.p, gccargs.len-1);
-1 looks like a bug.

[bytbox, 2012-02-23 17:00:02]

Hello golang-dev@googlegroups.com, 0xe2.0x9a.0x9b@gmail.com, ality@pbrane.org,
rsc@google.com (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2012-02-23 20:37:57]

LGTM

Thanks.

[rsc, 2012-02-23 20:38:09]

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

dist: treat CC as one unit

Fixes issue 3112.

R=golang-dev, 0xe2.0x9a.0x9b, ality, rsc, rsc
CC=golang-dev
http://codereview.appspot.com/5700044

Committer: Russ Cox <rsc@golang.org>

[atom, 2012-02-24 07:08:32]

On 2012/02/23 20:38:09, rsc wrote:
> *** Submitted as http://code.google.com/p/go/source/detail?r=a8b8f29a8442 ***
> 
> dist: treat CC as one unit
> 
> Fixes issue 3112.
> 
> R=golang-dev, 0xe2.0x9a.0x9b, ality, rsc, rsc
> CC=golang-dev
> http://codereview.appspot.com/5700044
> 
> Committer: Russ Cox <mailto:rsc@golang.org>

If CC="ccache clang", clang won't receive the additional -Wno-... arguments. See
http://code.google.com/p/go/source/browse/src/cmd/dist/build.c?name=a8b8f29a8...

[rsc1, 2012-02-24 15:30:38]

Thanks.  I'll fix it.

[rsc1, 2012-03-01 16:29:06]

On Fri, Feb 24, 2012 at 10:30, Russ Cox <rsc@google.com> wrote:
> Thanks.  I'll fix it.

http://codereview.appspot.com/5711052