Remove the intent to turn on Wextra for gcc (except MacOS)

Remove the intent to turn on Wextra for gcc (except MacOS).

We use clang with Wextra which seems to be more reliable and report less false positives. We no longer have an intent to turn on Wextra for gcc on posix-like systems any more (except MacOS).

[lgombos, 2013-11-09 15:41:55]

It seems the last attempt for Wextra was about 3 years ago. Perhaps we should
try with a subset first.

It seem that this subset does not actually requires any changes in the code.

commit 0fa17083628532ccc0941c9bc4ff13169d9c2c95
Author: evan@chromium.org
<evan@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date:   Fri Mar 26 00:48:05 2010 +0000

    Build fix.  Revert -Wextra.
    
    git-svn-id: svn://svn.chromium.org/chrome/trunk/src@42709
0039d316-1c4b-4281-b951-d872f2087c98

[Nico, 2013-11-09 18:58:24]

Does this find anything useful?

I believe clang doesn't have anything in -Wextra.

I think gcc emits -Wuninitialized warnings from its optimizer, which means they
vary with optimization level, which I'm not a fan of. (And -Wmaybe-uninitialized
has lots of false positives.)

The fixme is from the days before we had clang. I think we should just delete it
and rely on clang for strict diagnostics.

[lgombos, 2013-11-09 19:59:50]

On 2013/11/09 18:58:24, Nico (ooo until Nov 12) wrote:

> Does this find anything useful?

Main motivation for me was to turn on -Wunused-parameter for gcc.

Recent related discussion on blink-dev:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/_cpbUTuzJMo/zFXUKy03....

I do not have a strong desire to turn back on -Wextra, but I think
-Wunused-parameter should be on for gcc as well. I can drop Wuninitialized from
the CL if that is a dup/no-op.

[lgombos, 2013-11-09 20:31:11]

On 2013/11/09 19:59:50, lgombos wrote:
> On 2013/11/09 18:58:24, Nico (ooo until Nov 12) wrote:
> 
> > Does this find anything useful?
> 
> Main motivation for me was to turn on -Wunused-parameter for gcc.

Hoops, just noticed that right in the next line we disable -Wunused-parameter.

> The fixme is from the days before we had clang. I think we should just delete
it
> and rely on clang for strict diagnostics.

We have quite a bit of android specific code - e.g. the android-webview
directory where we do not use clang in production. 

How do we cover the android specific code for quality if we let gcc config fall
behind clang ?

[Nico, 2013-11-09 22:08:19]

On Sat, Nov 9, 2013 at 12:31 PM, <l.gombos@samsung.com> wrote:

> On 2013/11/09 19:59:50, lgombos wrote:
>
>> On 2013/11/09 18:58:24, Nico (ooo until Nov 12) wrote:
>>
>
>  > Does this find anything useful?
>>
>
>  Main motivation for me was to turn on -Wunused-parameter for gcc.
>>
>
> Hoops, just noticed that right in the next line we disable
> -Wunused-parameter.


The conclusion from the -Wunused-parameter thread seemed to be that we
don't want to use it.


> The fixme is from the days before we had clang. I think we should just
>> delete
>>
> it
>
>> and rely on clang for strict diagnostics.
>>
>
> We have quite a bit of android specific code - e.g. the android-webview
> directory where we do not use clang in production.
>

We don't need to use clang in production, just compiling the code with it
is enough. We do have android clang bots.


> How do we cover the android specific code for quality if we let gcc config
> fall
> behind clang ?
>
>
>
> https://codereview.chromium.org/61643017/
>

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

[lgombos, 2013-11-10 03:16:23]

On 2013/11/09 22:08:19, Nico (ooo until Nov 12) wrote:

> The conclusion from the -Wunused-parameter thread seemed to be that we
> don't want to use it.

Indeed. I removed the intent to turn on Wextra for gcc on posix-like platforms.

[Nico, 2013-11-10 03:41:08]

Thanks!

Can you repeat the title ("Remove the intent to turn on Wextra for gcc") as the
first line in the description? The cq only uses the description as commit
message. (Not sure why rietveld even has that field.)

https://codereview.chromium.org/61643017/diff/120001/build/common.gypi
File build/common.gypi (left):

https://codereview.chromium.org/61643017/diff/120001/build/common.gypi#oldcod...
build/common.gypi:2848: '-Wno-missing-field-initializers',
Are -Wno-unused-parameter and -Wno-missing-field-initializers not needed? Mac
below still has it for example. Is this turned off elsewhere? Is it not needed
if -Wextra isn't on?

[lgombos, 2013-11-10 04:16:28]

https://codereview.chromium.org/61643017/diff/120001/build/common.gypi
File build/common.gypi (left):

https://codereview.chromium.org/61643017/diff/120001/build/common.gypi#oldcod...
build/common.gypi:2848: '-Wno-missing-field-initializers',
On 2013/11/10 03:41:08, Nico (ooo until Nov 12) wrote:
> Are -Wno-unused-parameter and -Wno-missing-field-initializers not needed? Mac
> below still has it for example. Is this turned off elsewhere? Is it not needed
> if -Wextra isn't on?

The scope of this section is 'os_posix==1 and OS!="mac" and OS!="ios"'. Mac has
a section with -Wextra and -Wno-unused-parameter  turned on.

[Nico, 2013-11-10 04:23:57]

On 2013/11/10 04:16:28, lgombos wrote:
> https://codereview.chromium.org/61643017/diff/120001/build/common.gypi
> File build/common.gypi (left):
> 
>
https://codereview.chromium.org/61643017/diff/120001/build/common.gypi#oldcod...
> build/common.gypi:2848: '-Wno-missing-field-initializers',
> On 2013/11/10 03:41:08, Nico (ooo until Nov 12) wrote:
> > Are -Wno-unused-parameter and -Wno-missing-field-initializers not needed?
Mac
> > below still has it for example. Is this turned off elsewhere? Is it not
needed
> > if -Wextra isn't on?
> 
> The scope of this section is 'os_posix==1 and OS!="mac" and OS!="ios"'. Mac
has
> a section with -Wextra and -Wno-unused-parameter  turned on.

Right, the -Wextra is from the pre-clang days (I think we can remove that
maybe). I mean we probably want to keep -Wno-unused-parameter and
-Wno-missing-field-initializers around?

[lgombos, 2013-11-10 04:40:37]

On 2013/11/10 04:23:57, Nico (ooo until Nov 12) wrote:
> On 2013/11/10 04:16:28, lgombos wrote:
> > https://codereview.chromium.org/61643017/diff/120001/build/common.gypi
> > File build/common.gypi (left):
> > 
> >
>
https://codereview.chromium.org/61643017/diff/120001/build/common.gypi#oldcod...
> > build/common.gypi:2848: '-Wno-missing-field-initializers',
> > On 2013/11/10 03:41:08, Nico (ooo until Nov 12) wrote:
> > > Are -Wno-unused-parameter and -Wno-missing-field-initializers not needed?
> Mac
> > > below still has it for example. Is this turned off elsewhere? Is it not
> needed
> > > if -Wextra isn't on?
> > 
> > The scope of this section is 'os_posix==1 and OS!="mac" and OS!="ios"'. Mac
> has
> > a section with -Wextra and -Wno-unused-parameter  turned on.
> 
> Right, the -Wextra is from the pre-clang days (I think we can remove that
> maybe). I mean we probably want to keep -Wno-unused-parameter and
> -Wno-missing-field-initializers around?

For Mac we have that already somewhere else -
https://code.google.com/p/chromium/codesearch#chromium/src/build/common.gypi&...

For non-mac, -Wall does not turn on Wunused-parameter is not needed as we never
turn it on. Keeping it means no harm, but it looks to me as it is dead code. If
it would be not dead than I would expect some of the try bots to fail.

[lgombos, 2013-11-12 23:14:47]

On 2013/11/10 04:40:37, lgombos wrote:
> On 2013/11/10 04:23:57, Nico (ooo until Nov 12) wrote:
> > On 2013/11/10 04:16:28, lgombos wrote:
> > > https://codereview.chromium.org/61643017/diff/120001/build/common.gypi
> > > File build/common.gypi (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/61643017/diff/120001/build/common.gypi#oldcod...
> > > build/common.gypi:2848: '-Wno-missing-field-initializers',
> > > On 2013/11/10 03:41:08, Nico (ooo until Nov 12) wrote:
> > > > Are -Wno-unused-parameter and -Wno-missing-field-initializers not
needed?
> > Mac
> > > > below still has it for example. Is this turned off elsewhere? Is it not
> > needed
> > > > if -Wextra isn't on?
> > > 
> > > The scope of this section is 'os_posix==1 and OS!="mac" and OS!="ios"'.
Mac
> > has
> > > a section with -Wextra and -Wno-unused-parameter  turned on.
> > 
> > Right, the -Wextra is from the pre-clang days (I think we can remove that
> > maybe). I mean we probably want to keep -Wno-unused-parameter and
> > -Wno-missing-field-initializers around?
> 
> For Mac we have that already somewhere else -
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/common.gypi&...
> 
> For non-mac, -Wall does not turn on Wunused-parameter is not needed as we
never
> turn it on. Keeping it means no harm, but it looks to me as it is dead code.
If
> it would be not dead than I would expect some of the try bots to fail.

Any further feedback on this ?

[Zhenyao Mo, 2013-11-20 18:15:21]

On 2013/11/12 23:14:47, lgombos wrote:
> On 2013/11/10 04:40:37, lgombos wrote:
> > On 2013/11/10 04:23:57, Nico (ooo until Nov 12) wrote:
> > > On 2013/11/10 04:16:28, lgombos wrote:
> > > > https://codereview.chromium.org/61643017/diff/120001/build/common.gypi
> > > > File build/common.gypi (left):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/61643017/diff/120001/build/common.gypi#oldcod...
> > > > build/common.gypi:2848: '-Wno-missing-field-initializers',
> > > > On 2013/11/10 03:41:08, Nico (ooo until Nov 12) wrote:
> > > > > Are -Wno-unused-parameter and -Wno-missing-field-initializers not
> needed?
> > > Mac
> > > > > below still has it for example. Is this turned off elsewhere? Is it
not
> > > needed
> > > > > if -Wextra isn't on?
> > > > 
> > > > The scope of this section is 'os_posix==1 and OS!="mac" and OS!="ios"'.
> Mac
> > > has
> > > > a section with -Wextra and -Wno-unused-parameter  turned on.
> > > 
> > > Right, the -Wextra is from the pre-clang days (I think we can remove that
> > > maybe). I mean we probably want to keep -Wno-unused-parameter and
> > > -Wno-missing-field-initializers around?
> > 
> > For Mac we have that already somewhere else -
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/common.gypi&...
> > 
> > For non-mac, -Wall does not turn on Wunused-parameter is not needed as we
> never
> > turn it on. Keeping it means no harm, but it looks to me as it is dead code.
> If
> > it would be not dead than I would expect some of the try bots to fail.
> 
> Any further feedback on this ?

[Zhenyao Mo, 2013-11-20 18:18:03]

Right now the unsigned/signed mismatch only causes compiling fail on
android_aosp, which still uses gcc.

So the blink side CQ will let any unsigned/signed mismatch go through, but when
we try to roll into chromium, android_aosp CQ bot will fail.

We need to address this soon - this is adding unnecessary gardening cost.

[lgombos, 2013-12-02 04:13:12]

On 2013/11/20 18:18:03, Zhenyao Mo wrote:
> Right now the unsigned/signed mismatch only causes compiling fail on
> android_aosp, which still uses gcc.
> 
> So the blink side CQ will let any unsigned/signed mismatch go through, but
when
> we try to roll into chromium, android_aosp CQ bot will fail.
> 
> We need to address this soon - this is adding unnecessary gardening cost.

Zhanyao, would you be able to create a bug - and perhaps describes the scenario
better ?

I think blink_android_compile_dbg and blink_android_compile_rel should catch the
android_aosp failure as well, it is just the CQ does not run these bots for
blink.

Having said that, it might make sense to enable Wsign-compare for android for
gcc - as this is not enabled at the moment (I guess it is not enabled for
android_aosp either) - but maybe I am missing something.

[lgombos, 2013-12-02 04:34:15]

On 2013/12/02 04:13:12, lgombos wrote:

> Having said that, it might make sense to enable Wsign-compare for android for
> gcc - as this is not enabled at the moment (I guess it is not enabled for
> android_aosp either) - but maybe I am missing something.

Actually, I think Wsign-compare is not enabled for gcc for any platforms; these
are only reported with clang.

[lgombos, 2014-01-05 04:21:28]

On 2013/12/02 04:34:15, lgombos wrote:
> On 2013/12/02 04:13:12, lgombos wrote:
> 
> > Having said that, it might make sense to enable Wsign-compare for android
for
> > gcc - as this is not enabled at the moment (I guess it is not enabled for
> > android_aosp either) - but maybe I am missing something.
> 
> Actually, I think Wsign-compare is not enabled for gcc for any platforms;
these
> are only reported with clang.

Any feedback on this ?

[lgombos, 2014-07-04 19:06:46]

ptal

https://codereview.chromium.org/61643017/diff/120001/build/common.gypi
File build/common.gypi (left):

https://codereview.chromium.org/61643017/diff/120001/build/common.gypi#oldcod...
build/common.gypi:2848: '-Wno-missing-field-initializers',
On 2013/11/10 03:41:08, Nico (away) wrote:
> Are -Wno-unused-parameter and -Wno-missing-field-initializers not needed? Mac
> below still has it for example. Is this turned off elsewhere? Is it not needed
> if -Wextra isn't on?

It is the latter - it is not needed if -Wextra isn't on.

[lgombos, 2015-03-23 23:30:01]

Nico, can you please take a look ? 

This is just removing some TODO for something that we no longer plan to do to
avoid confusion.

[Nico, 2015-03-23 23:30:45]

lgtm

[lgombos, 2015-03-23 23:41:27]

The CQ bit was checked by l.gombos@samsung.com

[lgombos, 2015-03-24 01:18:40]

The CQ bit was unchecked by l.gombos@samsung.com

[lgombos, 2015-03-24 01:18:49]

The CQ bit was checked by l.gombos@samsung.com

[lgombos, 2015-03-24 13:24:02]

The CQ bit was unchecked by l.gombos@samsung.com

[lgombos, 2015-03-24 13:24:15]

The CQ bit was checked by l.gombos@samsung.com

[Evan Martin, 2015-03-24 17:19:03]

LGTM

On Mon, Mar 23, 2015 at 4:30 PM, <l.gombos@samsung.com> wrote:

> Reviewers: Nico, Evan Martin,
>
> Message:
> Nico, can you please take a look ?
>
> This is just removing some TODO for something that we no longer plan to do
> to
> avoid confusion.
>
>
> Description:
> Remove the intent to turn on Wextra for gcc.
>
> We use clang with Wextra which seems to be more reliable and report less
> false
> positives. We no longer have an intent to turn on Wextra for gcc on
> posix-like
> systems any more.
>
>
> Please review this at https://codereview.chromium.org/61643017/
>
> Base URL: http://git.chromium.org/chromium/src.git@master
>
> Affected files (+0, -2 lines):
>   M build/common.gypi
>
>
> Index: build/common.gypi
> diff --git a/build/common.gypi b/build/common.gypi
> index 745035c5503f8c5fb9c85b8c4408d4b49b142826..
> d02cf11c2aff683b3ecdea021e1b9cb9c07f6141 100644
> --- a/build/common.gypi
> +++ b/build/common.gypi
> @@ -3603,8 +3603,6 @@
>            '-pthread',
>            '-fno-strict-aliasing',  # See http://crbug.com/32204
>            '-Wall',
> -          # TODO(evan): turn this back on once all the builds work.
> -          # '-Wextra',
>            # Don't warn about unused function params.  We use those
> everywhere.
>            '-Wno-unused-parameter',
>            # Don't warn about the "struct foo f = {0};" initialization
> pattern.
>
>
>

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

[lgombos, 2015-03-24 21:28:33]

The CQ bit was unchecked by l.gombos@samsung.com

[lgombos, 2015-03-24 21:51:51]

Trying to land this CL as https://codereview.chromium.org/1029953003/ . Closing
this issue.