gn/linux: warnings as errors, turn on Wextra

gn/linux: warnings as errors, turn on Wextra

Also remove a few redundant consts that result in warnings like:

../../content/common/cursors/webcursor_aurax11.cc:20:1: error: 'const' type qualifier on return type has no effect [-Werror,-Wignored-qualifiers]
const ui::PlatformCursor WebCursor::GetPlatformCursor() {
^~~~~~


TBR=davemoore@chromium.org,sadrul@chromium.org
R=brettw@chromium.org, thakis@chromium.org,rch@chromium.org
BUG=393046, 335824
Committed: https://crrev.com/4c2d33ac8c061cf27a594473d6f9a450ee6c3c89
Cr-Commit-Position: refs/heads/master@{#293930}

[scottmg, 2014-09-05 01:46:14]

scottmg@chromium.org changed reviewers:
+ brettw@chromium.org

[Nico, 2014-09-05 02:00:52]

On 2014/09/05 01:48:27, scottmg wrote:

We want this always, not just for chromium_code, right?

[scottmg, 2014-09-05 20:58:42]

Patchset #2 (id:20001) has been deleted

[scottmg, 2014-09-05 20:58:57]

Patchset #2 (id:20001) has been deleted

[scottmg, 2014-09-05 21:02:19]

On 2014/09/05 02:00:52, Nico (hiding) wrote:
> On 2014/09/05 01:48:27, scottmg wrote:
> 
> We want this always, not just for chromium_code, right?

Sure, why not! Turned on Wextra too, see if that flies.

[Nico, 2014-09-05 21:05:02]

lgtm

https://codereview.chromium.org/538333002/diff/40001/build/config/compiler/BU...
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/538333002/diff/40001/build/config/compiler/BU...
build/config/compiler/BUILD.gn:483: "-Wextra",
(in think in clang, wextra doesn't add anything, so this is probably safe)

[Nico, 2014-09-05 21:05:37]

…looks like icu.gn is missing a -Wno-deprecated

[Nico, 2014-09-05 21:05:45]

*icu/BUILD.gn

[brettw, 2014-09-05 21:55:56]

lgtm

[Nico, 2014-09-08 20:51:53]

https://codereview.chromium.org/538333002/diff/120001/build/secondary/third_p...
File build/secondary/third_party/icu/BUILD.gn (right):

https://codereview.chromium.org/538333002/diff/120001/build/secondary/third_p...
build/secondary/third_party/icu/BUILD.gn:269: "-Wno-deprecated-declarations",
the gyp files have short explanations for suppressed warnings (in this case "#
ICU uses its own deprecated functions."). Might be worth keeping?

[scottmg, 2014-09-08 20:55:55]

thanks

https://codereview.chromium.org/538333002/diff/120001/build/secondary/third_p...
File build/secondary/third_party/icu/BUILD.gn (right):

https://codereview.chromium.org/538333002/diff/120001/build/secondary/third_p...
build/secondary/third_party/icu/BUILD.gn:269: "-Wno-deprecated-declarations",
On 2014/09/08 20:51:52, Nico (hiding) wrote:
> the gyp files have short explanations for suppressed warnings (in this case "#
> ICU uses its own deprecated functions."). Might be worth keeping?

Done.

[Nico, 2014-09-08 21:00:23]

still lgtm

Ship it!

https://codereview.chromium.org/538333002/diff/160001/build/secondary/third_p...
File build/secondary/third_party/libsrtp/BUILD.gn (right):

https://codereview.chromium.org/538333002/diff/160001/build/secondary/third_p...
build/secondary/third_party/libsrtp/BUILD.gn:216: cflags = [
"-Wno-implicit-function-declaration" ]
Hm, gyp doesn't seem to need this.

[scottmg, 2014-09-08 21:01:48]

scottmg@chromium.org changed reviewers:
+ davemoore@chromium.org, resleevi@chromium.org, sadrul@chromium.org

[scottmg, 2014-09-08 21:01:49]

Adding a bunch of tbrs for removal of unnecessary 'const's:

davemoore:
  ash/shelf/
  chrome/browser/ui/ash/launcher/

brettw:
  content/

rsleevi:
  net/

sadrul:
  ui/keyboard/

[scottmg, 2014-09-08 21:02:10]

scottmg@chromium.org changed reviewers:
+ rsleevi@chromium.org
- resleevi@chromium.org

[scottmg, 2014-09-08 21:02:21]

ahem, "rsleevi"

[scottmg, 2014-09-08 21:02:28]

The CQ bit was checked by scottmg@chromium.org

[scottmg, 2014-09-08 21:05:05]

https://codereview.chromium.org/538333002/diff/160001/build/secondary/third_p...
File build/secondary/third_party/libsrtp/BUILD.gn (right):

https://codereview.chromium.org/538333002/diff/160001/build/secondary/third_p...
build/secondary/third_party/libsrtp/BUILD.gn:216: cflags = [
"-Wno-implicit-function-declaration" ]
On 2014/09/08 21:00:23, Nico (hiding) wrote:
> Hm, gyp doesn't seem to need this.

Hmm, I just tried again to quote an error... and neither does gn (now?). I don't
think I added it for fun, but, uh, will remove anyway.

[scottmg, 2014-09-08 21:05:12]

The CQ bit was unchecked by scottmg@chromium.org

[Ryan Sleevi, 2014-09-08 21:05:37]

rsleevi@chromium.org changed reviewers:
+ rch@chromium.org
- rsleevi@chromium.org

[Ryan Sleevi, 2014-09-08 21:05:38]

-me
+rch

Not sure if this is the "QUIC code that is third party" or "QUIC code that
isn't". Ryan#1 would know.

[scottmg, 2014-09-08 21:05:55]

The CQ bit was checked by scottmg@chromium.org

[scottmg, 2014-09-08 21:09:42]

The CQ bit was unchecked by scottmg@chromium.org

[scottmg, 2014-09-08 21:11:29]

On 2014/09/08 21:05:05, scottmg wrote:
>
https://codereview.chromium.org/538333002/diff/160001/build/secondary/third_p...
> File build/secondary/third_party/libsrtp/BUILD.gn (right):
> 
>
https://codereview.chromium.org/538333002/diff/160001/build/secondary/third_p...
> build/secondary/third_party/libsrtp/BUILD.gn:216: cflags = [
> "-Wno-implicit-function-declaration" ]
> On 2014/09/08 21:00:23, Nico (hiding) wrote:
> > Hm, gyp doesn't seem to need this.
> 
> Hmm, I just tried again to quote an error... and neither does gn (now?). I
don't
> think I added it for fun, but, uh, will remove anyway.

Bleh, scratch that, looking at wrong place.

../../third_party/libsrtp/srtp/test/rtpw.c:269:22: error: implicit declaration
of function 'inet_addr' is invalid in C99
[-Werror,-Wimplicit-function-declaration]
  rcvr_addr.s_addr = inet_addr(address);
                     ^

I'm not sure why. Maybe a different compiler flag so we get c99 instead of c90
here?

[scottmg, 2014-09-08 21:12:10]

Patchset #9 (id:180001) has been deleted

[scottmg, 2014-09-08 21:13:06]

Patchset #9 (id:200001) has been deleted

[Nico, 2014-09-08 21:19:49]

On 2014/09/08 21:11:29, scottmg wrote:
> On 2014/09/08 21:05:05, scottmg wrote:
> >
>
https://codereview.chromium.org/538333002/diff/160001/build/secondary/third_p...
> > File build/secondary/third_party/libsrtp/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/538333002/diff/160001/build/secondary/third_p...
> > build/secondary/third_party/libsrtp/BUILD.gn:216: cflags = [
> > "-Wno-implicit-function-declaration" ]
> > On 2014/09/08 21:00:23, Nico (hiding) wrote:
> > > Hm, gyp doesn't seem to need this.
> > 
> > Hmm, I just tried again to quote an error... and neither does gn (now?). I
> don't
> > think I added it for fun, but, uh, will remove anyway.
> 
> Bleh, scratch that, looking at wrong place.
> 
> ../../third_party/libsrtp/srtp/test/rtpw.c:269:22: error: implicit declaration
> of function 'inet_addr' is invalid in C99
> [-Werror,-Wimplicit-function-declaration]
>   rcvr_addr.s_addr = inet_addr(address);
>                      ^
> 
> I'm not sure why. Maybe a different compiler flag so we get c99 instead of c90
> here?

Not sure; clang defaults to c99. Maybe something explicitly asks for 90?
https://code.google.com/p/chromium/codesearch#search/&q=c90%20file:gyp&sq=pac...
… no. much confuse!

[Ryan Hamilton, 2014-09-08 21:31:43]

net/tools/quic: LGTM

[scottmg, 2014-09-08 21:36:28]

On 2014/09/08 21:19:49, Nico (hiding) wrote:
> On 2014/09/08 21:11:29, scottmg wrote:
> > On 2014/09/08 21:05:05, scottmg wrote:
> > >
> >
>
https://codereview.chromium.org/538333002/diff/160001/build/secondary/third_p...
> > > File build/secondary/third_party/libsrtp/BUILD.gn (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/538333002/diff/160001/build/secondary/third_p...
> > > build/secondary/third_party/libsrtp/BUILD.gn:216: cflags = [
> > > "-Wno-implicit-function-declaration" ]
> > > On 2014/09/08 21:00:23, Nico (hiding) wrote:
> > > > Hm, gyp doesn't seem to need this.
> > > 
> > > Hmm, I just tried again to quote an error... and neither does gn (now?). I
> > don't
> > > think I added it for fun, but, uh, will remove anyway.
> > 
> > Bleh, scratch that, looking at wrong place.
> > 
> > ../../third_party/libsrtp/srtp/test/rtpw.c:269:22: error: implicit
declaration
> > of function 'inet_addr' is invalid in C99
> > [-Werror,-Wimplicit-function-declaration]
> >   rcvr_addr.s_addr = inet_addr(address);
> >                      ^
> > 
> > I'm not sure why. Maybe a different compiler flag so we get c99 instead of
c90
> > here?
> 
> Not sure; clang defaults to c99. Maybe something explicitly asks for 90?
>
https://code.google.com/p/chromium/codesearch#search/&q=c90%20file:gyp&sq=pac...
> … no. much confuse!

https://codereview.chromium.org/551923003 reports some diffs, but none look
important.

  dash_f differ:
    In gyp, but not in GN:
      -fno-exceptions
      -fno-slp-vectorize
    In GN, but not in gyp:
      -fno-ident

  defines differ:
    In gyp, but not in GN:
      -DCLD2_DATA_SOURCE=static
      -DCR_CLANG_REVISION=214024
      -DDYNAMIC_ANNOTATIONS_ENABLED=0
      -DNDEBUG
      -DNVALGRIND
    In GN, but not in gyp:
      -DDISABLE_NACL

  include_dirs differ:
    In gyp, but not in GN:
      
    In GN, but not in gyp:
      -I../..

  warnings differ:
    In gyp, but not in GN:
      -Wno-reserved-user-defined-literal
      -Wno-unneeded-internal-declaration
    In GN, but not in gyp:
      -Wendif-labels
      -Wno-implicit-function-declaration
      -Wno-unused-function

  other differ:
    In gyp, but not in GN:
      -B/work/cr/src/third_party/binutils/Linux_x64/Release/bin
      -Xclang
/work/cr/src/tools/clang/scripts/../../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so
      -march=x86-64
    In GN, but not in gyp:
      -Xclang
../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so
      -g2

...

Oh. Running the output of the -t commands for both versions results in the same
output for both gn and gyp (that is, there's a warning).

In gyp, that target just isn't built by 'all', I think.

[scottmg, 2014-09-08 21:38:37]

https://codereview.chromium.org/538333002/diff/160001/build/secondary/third_p...
File build/secondary/third_party/libsrtp/BUILD.gn (right):

https://codereview.chromium.org/538333002/diff/160001/build/secondary/third_p...
build/secondary/third_party/libsrtp/BUILD.gn:151: # TODO(GYP): A bunch of these
tests don't compile (in gyp either). They're
It would seem I already knew this. But apparently it's slightly true on Linux
too.

[scottmg, 2014-09-08 21:38:46]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2014-09-08 21:40:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/scottmg@chromium.org/538333002/160001

[Nico, 2014-09-08 21:40:46]

On Mon, Sep 8, 2014 at 2:36 PM, <scottmg@chromium.org> wrote:

> On 2014/09/08 21:19:49, Nico (hiding) wrote:
>
>> On 2014/09/08 21:11:29, scottmg wrote:
>> > On 2014/09/08 21:05:05, scottmg wrote:
>> > >
>> >
>>
>
> https://codereview.chromium.org/538333002/diff/160001/
> build/secondary/third_party/libsrtp/BUILD.gn
>
>> > > File build/secondary/third_party/libsrtp/BUILD.gn (right):
>> > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/538333002/diff/160001/
> build/secondary/third_party/libsrtp/BUILD.gn#newcode216
>
>> > > build/secondary/third_party/libsrtp/BUILD.gn:216: cflags = [
>> > > "-Wno-implicit-function-declaration" ]
>> > > On 2014/09/08 21:00:23, Nico (hiding) wrote:
>> > > > Hm, gyp doesn't seem to need this.
>> > >
>> > > Hmm, I just tried again to quote an error... and neither does gn
>> (now?). I
>> > don't
>> > > think I added it for fun, but, uh, will remove anyway.
>> >
>> > Bleh, scratch that, looking at wrong place.
>> >
>> > ../../third_party/libsrtp/srtp/test/rtpw.c:269:22: error: implicit
>>
> declaration
>
>> > of function 'inet_addr' is invalid in C99
>> > [-Werror,-Wimplicit-function-declaration]
>> >   rcvr_addr.s_addr = inet_addr(address);
>> >                      ^
>> >
>> > I'm not sure why. Maybe a different compiler flag so we get c99 instead
>> of
>>
> c90
>
>> > here?
>>
>
>  Not sure; clang defaults to c99. Maybe something explicitly asks for 90?
>>
>
> https://code.google.com/p/chromium/codesearch#search/&q=
> c90%20file:gyp&sq=package:chromium&type=cs
>
>> ... no. much confuse!
>>
>
> https://codereview.chromium.org/551923003 reports some diffs, but none
> look
> important.
>
>   dash_f differ:
>     In gyp, but not in GN:
>       -fno-exceptions

      -fno-slp-vectorize
>

^ this one is very important, the clang revision we have produces broken
code without this (fixed on clang trunk, and we'll remove the flag after
the next clang roll, but for now it's)…oh, you probably meant "not
important for the warning". Yes.


>     In GN, but not in gyp:
>       -fno-ident
>
>   defines differ:
>     In gyp, but not in GN:
>       -DCLD2_DATA_SOURCE=static
>       -DCR_CLANG_REVISION=214024
>       -DDYNAMIC_ANNOTATIONS_ENABLED=0
>       -DNDEBUG
>       -DNVALGRIND
>     In GN, but not in gyp:
>       -DDISABLE_NACL
>
>   include_dirs differ:
>     In gyp, but not in GN:
>
>     In GN, but not in gyp:
>       -I../..
>
>   warnings differ:
>     In gyp, but not in GN:
>       -Wno-reserved-user-defined-literal
>       -Wno-unneeded-internal-declaration
>     In GN, but not in gyp:
>       -Wendif-labels
>       -Wno-implicit-function-declaration
>       -Wno-unused-function
>
>   other differ:
>     In gyp, but not in GN:
>       -B/work/cr/src/third_party/binutils/Linux_x64/Release/bin
>       -Xclang
> /work/cr/src/tools/clang/scripts/../../../third_party/
> llvm-build/Release+Asserts/lib/libFindBadConstructs.so
>       -march=x86-64
>     In GN, but not in gyp:
>       -Xclang
> ../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so
>       -g2
>
> ...
>
> Oh. Running the output of the -t commands for both versions results in the
> same
> output for both gn and gyp (that is, there's a warning).
>
> In gyp, that target just isn't built by 'all', I think.
>
> https://codereview.chromium.org/538333002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[scottmg, 2014-09-08 21:45:08]

On Mon, Sep 8, 2014 at 2:40 PM, Nico Weber <thakis@chromium.org> wrote:

> On Mon, Sep 8, 2014 at 2:36 PM, <scottmg@chromium.org> wrote:
>
>> On 2014/09/08 21:19:49, Nico (hiding) wrote:
>>
>>> On 2014/09/08 21:11:29, scottmg wrote:
>>> > On 2014/09/08 21:05:05, scottmg wrote:
>>> > >
>>> >
>>>
>>
>> https://codereview.chromium.org/538333002/diff/160001/
>> build/secondary/third_party/libsrtp/BUILD.gn
>>
>>> > > File build/secondary/third_party/libsrtp/BUILD.gn (right):
>>> > >
>>> > >
>>> >
>>>
>>
>> https://codereview.chromium.org/538333002/diff/160001/
>> build/secondary/third_party/libsrtp/BUILD.gn#newcode216
>>
>>> > > build/secondary/third_party/libsrtp/BUILD.gn:216: cflags = [
>>> > > "-Wno-implicit-function-declaration" ]
>>> > > On 2014/09/08 21:00:23, Nico (hiding) wrote:
>>> > > > Hm, gyp doesn't seem to need this.
>>> > >
>>> > > Hmm, I just tried again to quote an error... and neither does gn
>>> (now?). I
>>> > don't
>>> > > think I added it for fun, but, uh, will remove anyway.
>>> >
>>> > Bleh, scratch that, looking at wrong place.
>>> >
>>> > ../../third_party/libsrtp/srtp/test/rtpw.c:269:22: error: implicit
>>>
>> declaration
>>
>>> > of function 'inet_addr' is invalid in C99
>>> > [-Werror,-Wimplicit-function-declaration]
>>> >   rcvr_addr.s_addr = inet_addr(address);
>>> >                      ^
>>> >
>>> > I'm not sure why. Maybe a different compiler flag so we get c99
>>> instead of
>>>
>> c90
>>
>>> > here?
>>>
>>
>>  Not sure; clang defaults to c99. Maybe something explicitly asks for 90?
>>>
>>
>> https://code.google.com/p/chromium/codesearch#search/&q=
>> c90%20file:gyp&sq=package:chromium&type=cs
>>
>>> ... no. much confuse!
>>>
>>
>> https://codereview.chromium.org/551923003 reports some diffs, but none
>> look
>> important.
>>
>>   dash_f differ:
>>     In gyp, but not in GN:
>>       -fno-exceptions
>
>       -fno-slp-vectorize
>>
>
> ^ this one is very important, the clang revision we have produces broken
> code without this (fixed on clang trunk, and we'll remove the flag after
> the next clang roll, but for now it's)…oh, you probably meant "not
> important for the warning". Yes.
>

Yeah, that. :)

(Regarding the slp-vectorize, I can confirm after seeing it had landed
upstream and not bother to check clang revision we're building against
first that we do indeed still need it :)
https://codereview.chromium.org/541963002/ )


>
>
>>     In GN, but not in gyp:
>>       -fno-ident
>>
>>   defines differ:
>>     In gyp, but not in GN:
>>       -DCLD2_DATA_SOURCE=static
>>       -DCR_CLANG_REVISION=214024
>>       -DDYNAMIC_ANNOTATIONS_ENABLED=0
>>       -DNDEBUG
>>       -DNVALGRIND
>>     In GN, but not in gyp:
>>       -DDISABLE_NACL
>>
>>   include_dirs differ:
>>     In gyp, but not in GN:
>>
>>     In GN, but not in gyp:
>>       -I../..
>>
>>   warnings differ:
>>     In gyp, but not in GN:
>>       -Wno-reserved-user-defined-literal
>>       -Wno-unneeded-internal-declaration
>>     In GN, but not in gyp:
>>       -Wendif-labels
>>       -Wno-implicit-function-declaration
>>       -Wno-unused-function
>>
>>   other differ:
>>     In gyp, but not in GN:
>>       -B/work/cr/src/third_party/binutils/Linux_x64/Release/bin
>>       -Xclang
>> /work/cr/src/tools/clang/scripts/../../../third_party/
>> llvm-build/Release+Asserts/lib/libFindBadConstructs.so
>>       -march=x86-64
>>     In GN, but not in gyp:
>>       -Xclang
>> ../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so
>>       -g2
>>
>> ...
>>
>> Oh. Running the output of the -t commands for both versions results in
>> the same
>> output for both gn and gyp (that is, there's a warning).
>>
>> In gyp, that target just isn't built by 'all', I think.
>>
>> https://codereview.chromium.org/538333002/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[scottmg, 2014-09-08 23:58:53]

The CQ bit was unchecked by scottmg@chromium.org

[scottmg, 2014-09-08 23:59:07]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2014-09-09 00:28:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/scottmg@chromium.org/538333002/160001

[commit-bot: I haz the power, 2014-09-09 02:22:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-09 02:22:46]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[scottmg, 2014-09-09 03:56:41]

Patchset #9 (id:220001) has been deleted

[scottmg, 2014-09-09 03:56:47]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2014-09-09 04:11:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/scottmg@chromium.org/538333002/240001

[commit-bot: I haz the power, 2014-09-09 05:44:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-09 05:44:14]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nico, 2014-09-09 05:52:43]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2014-09-09 05:53:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/scottmg@chromium.org/538333002/240001

[commit-bot: I haz the power, 2014-09-09 06:28:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-09 06:28:31]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Paweł Hajdan Jr., 2014-09-09 12:34:11]

The CQ bit was checked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2014-09-09 12:34:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/scottmg@chromium.org/538333002/240001

[commit-bot: I haz the power, 2014-09-09 13:16:08]

Committed patchset #9 (id:240001) as de3316e7a276d3b47b4d00798f5327370d585378

[commit-bot: I haz the power, 2014-09-10 03:53:05]

Patchset 9 (id:??) landed as
https://crrev.com/4c2d33ac8c061cf27a594473d6f9a450ee6c3c89
Cr-Commit-Position: refs/heads/master@{#293930}