code review 11610044: cmd/dist, cmd/go: enable more warnings, make clang erro...

cmd/dist, cmd/go: enable more warnings, make clang errors legible

This does not change the default compiler on OS X to clang.
It appears that for now we can keep using gcc as long as we
enable a few more warning settings that are on-by-default
elsewhere.

[rsc, 2013-07-19 23:28:42]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/

[bradfitz, 2013-07-19 23:29:40]

LGTM
 On Jul 20, 2013 9:28 AM, <rsc@golang.org> 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/dist, cmd/go: enable more warnings, make clang errors legible
>
> This does not change the default compiler on OS X to clang.
> It appears that for now we can keep using gcc as long as we
> enable a few more warning settings that are on-by-default
> elsewhere.
>
> Please review this at
https://codereview.appspot.**com/11610044/<https://codereview.appspot.com/116...
>
> Affected files:
>   M src/cmd/dist/build.c
>   M src/cmd/dist/unix.c
>   M src/cmd/go/build.go
>   M src/cmd/go/env.go
>
>
> Index: src/cmd/dist/build.c
> ==============================**==============================**=======
> --- a/src/cmd/dist/build.c
> +++ b/src/cmd/dist/build.c
> @@ -407,12 +407,16 @@
>         // native Plan 9 compilers don't like non-standard prototypes
>         // so let gcc catch them.
>         "-Wstrict-prototypes",
> +       "-Wextra",
> +       "-Wunused",
> +       "-Wuninitialized",
>         "-Wno-sign-compare",
>         "-Wno-missing-braces",
>         "-Wno-parentheses",
>         "-Wno-unknown-pragmas",
>         "-Wno-switch",
>         "-Wno-comment",
> +       "-Wno-missing-field-**initializers",
>         "-Werror",
>         "-fno-common",
>         "-ggdb",
> @@ -620,6 +624,8 @@
>                 for(i=0; i<nelem(proto_gccargs); i++)
>                         vadd(&gccargs, proto_gccargs[i]);
>                 if(clang) {
> +                       // disable ASCII art in clang errors, if possible
> +                       vadd(&gccargs, "-fno-caret-diagnostics");
>                         // clang is too smart about unused command-line
> arguments
>                         vadd(&gccargs, "-Qunused-arguments");
>                 }
> Index: src/cmd/dist/unix.c
> ==============================**==============================**=======
> --- a/src/cmd/dist/unix.c
> +++ b/src/cmd/dist/unix.c
> @@ -656,6 +656,8 @@
>         setvbuf(stdout, nil, _IOLBF, 0);
>         setvbuf(stderr, nil, _IOLBF, 0);
>
> +       setenv("TERM", "dumb", 1); // disable escape codes in clang errors
> +
>         binit(&b);
>
>         slash = "/";
> Index: src/cmd/go/build.go
> ==============================**==============================**=======
> --- a/src/cmd/go/build.go
> +++ b/src/cmd/go/build.go
> @@ -1811,8 +1811,10 @@
>                 }
>         }
>
> -       // clang is too smart about command-line arguments
>         if strings.Contains(a[0], "clang") {
> +               // disable ASCII art in clang errors, if possible
> +               a = append(a, "-fno-caret-diagnostics")
> +               // clang is too smart about command-line arguments
>                 a = append(a, "-Qunused-arguments")
>         }
>
> Index: src/cmd/go/env.go
> ==============================**==============================**=======
> --- a/src/cmd/go/env.go
> +++ b/src/cmd/go/env.go
> @@ -45,6 +45,9 @@
>                 {"GORACE", os.Getenv("GORACE")},
>                 {"GOROOT", goroot},
>                 {"GOTOOLDIR", toolDir},
> +
> +               // disable escape codes in clang errors
> +               {"TERM", "dumb"},
>         }
>
>         if goos != "plan9" {
>
>
> --
>
> ---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...
> .
>
>
>

[dave_cheney.net, 2013-07-19 23:35:46]

LGTM.

[rsc, 2013-07-19 23:36:18]

*** Submitted as https://code.google.com/p/go/source/detail?r=1b4c0f6479cb ***

cmd/dist, cmd/go: enable more warnings, make clang errors legible

This does not change the default compiler on OS X to clang.
It appears that for now we can keep using gcc as long as we
enable a few more warning settings that are on-by-default
elsewhere.

R=golang-dev, bradfitz, dave
CC=golang-dev
https://codereview.appspot.com/11610044

[remyoudompheng, 2013-07-21 09:13:50]

https://codereview.appspot.com/11610044/diff/8001/src/cmd/dist/build.c
File src/cmd/dist/build.c (right):

https://codereview.appspot.com/11610044/diff/8001/src/cmd/dist/build.c#newcod...
src/cmd/dist/build.c:628: vadd(&gccargs, "-fno-caret-diagnostics");
Any reason why we don't do the same for GCC? (-fno-diagnostics-show-caret)

[iant, 2013-07-22 23:40:25]

FYI

https://codereview.appspot.com/11610044/diff/3001/src/cmd/dist/build.c
File src/cmd/dist/build.c (right):

https://codereview.appspot.com/11610044/diff/3001/src/cmd/dist/build.c#newcod...
src/cmd/dist/build.c:628: vadd(&gccargs, "-fno-caret-diagnostics");
GCC will also emit caret diagnostics by default as of 4.8.  Turning them off
requires the -fno-diagnostics-show-caret option.  Unfortunately that option does
not work for GCC versions prior to 4.8.

[rsc, 2013-07-22 23:51:20]

Is there a "ignore unknown -f options" option?

Russ

[iant, 2013-07-23 00:07:18]

On Mon, Jul 22, 2013 at 4:51 PM, Russ Cox <rsc@golang.org> wrote:
> Is there a "ignore unknown -f options" option?

Unfortunately not.

Ian