Use chromium warning levels for blink

[Laszlo Gombos]

Hi,

It seems that warnings are not treated as errors for blink code in any of the build configurations (with the exception of web/ directory). Chromium code treats warnings as errors (with an option to overwrite the default policy in a gyp variable) for certain build configurations (as an example linux release build - for details see build/common.gypi).

I think blink should also treat warnings as errors and match the chromium warning level, which would mean as an example that unused variables would cause a build error.

Any thoughts or concern with this directions ? 

Related CL -  https://codereview.chromium.org/47623002/

Thanks,

  Laszlo

[Christophe Dumez]

+1 for treating warnings as errors in Blink, similarly to Chromium.

Kr,

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

--
Christophe DUMEZ

[Nico Weber]

Whoa, we apparently lost this in https://codereview.chromium.org/14110002 (found via ` git log -SWerror`). No cookie for the reviewer of that CL :-/

We should definitely turn this on again, but we should stick to the default warning set as determined by build/common.gypi and fix all warnings before turning it on. (Gating it on OS for now is fine.) It's probably best to start with a clang configuration for this, as it has higher-quality warnings and since all bots and developers will use the same version. Can you file a tracking bug for this?

Nico

[Laszlo Gombos]

Hi Nico,

Tracking bug - https://code.google.com/p/chromium/issues/detail?id=312287

My plan of attack was to land https://codereview.chromium.org/47623002 (turns Werror on for gcc on linux and android), then fix unused-but-set-variable warnings which would than enable to remove the -Wno-unused-but-set-variable flag (which apparently clang does not understand).

This would than enable to turn on Werror for clang as well.

Regards,

  Laszlo

[Nico Weber]

Looking through build/common.gypi, -Werror should always be on as far as I can tell. Maybe you set "werror" in your ~/.gyp/include.gypi ? Or your target_arch is mipsel or are building for chromeos?

(But restoring -Wall is still very important.)

[Nico Weber]

A short update on this: Warnings are now again enabled on all platforms except Windows, so we're now back to where we were a few months ago.

Major thanks to Laszlo for finding this problem and for fixing hundreds of warnings, which was required to turn -Wall -Werror back on. We found and fixed several code and infrastructure bugs along the way (did you known that blink_android_compile_dbg used to do release builds?). Resolving this required editing over 170 source files.

[Peter Kasting]

On Tue, Nov 5, 2013 at 4:02 PM, Nico Weber <tha...@chromium.org> wrote:

A short update on this: Warnings are now again enabled on all platforms except Windows, so we're now back to where we were a few months ago.

Awesome!

Is there a plan for enabling warnings on Windows?  Is engineering help needed in this area?

PK

[Nico Weber]

I don't think anyone is looking at Windows. It mostly requires editing Source/config.gyp and moving the "'chromium_code': 1" variables block out of the conditional bit and then fixing all the errors. Maybe it won't be too bad, since the code is warning-free with two other compilers. Help would definitely be appreciated!

[Steve Block]

On a related note, I think that WebKit used to use -Wunused-parameter.
See, for example, the ASSERT_UNUSED() in
Document::didAssociateFormControlsTimerFired(). It seems that we no
longer specify this flag for non-test Blink code (build/common.gypi
specifies -Wextra on Mac, but overrides this with -Wunused-parameter
on all platforms).

Was this an intentional decision, or should we re-enable
-Wunused-parameter for Blink?

Steve

[Adam Barth]

I don't believe Chromium enables -Wunused-parameter.  IMHO, we should converge with Chromium's compile settings.

Adam