Clang MSVC Warning Harmonization |
|||||||
Issue descriptionThere are a host of MSVC warnings that currently are not caught by clang. Enabling the equivalent warning on clang will go a long way towards making the code better and making MSVC compatibility easier. A few of these include C4200 - nonstandard extension used: zero-sized array in struct/union C4201 - nonstandard extension used: nameless struct/union C4204 - nonstandard extension used : non-constant aggregate initializer C4221 - nonstandard extension used : 'identifier' : cannot be initialized using address of automatic variable C4245 - 'conversion' : conversion from 'type1' to 'type2', signed/unsigned mismatch C4267 - 'var' : conversion from 'size_t' to 'type', possible loss of data C4389 - 'operator' : signed/unsigned mismatch C4661 - 'identifier' : no suitable definition provided for explicit template instantiation request C4701 - Potentially uninitialized local variable 'name' used C4703 - Potentially uninitialized local pointer variable 'name' used C4715 - 'function' : not all control paths return a value' An approach here is the following 1) Bring MSVC to Clang's level of checking 2) For a single disabled warning, find the equivalent Clang flag. 3) Fix the codebase for that Clang flag. 4) Enable that Clang flag by default. 5) Remove the MSVC warning disable flag. ⛆ |
|
|
,
Jun 2 2018
(but I think working on the wconversion and wunreachable-code bugs is useful, and I do believe that many warnings are useful!)
,
Jun 4 2018
Yeah, we can probably disable the nonstandard extension warnings. The list above is derived from a global build of MSVC near tip of trunk.
,
Jun 4 2018
-Wunreachable-code bug 346399 -Wconversion bug 588506
,
Jun 6 2018
,
Jun 6 2018
I agree that we should try to keep the warnings roughly consistent between compilers, which in some cases means disabling VC++ warnings that are not helpful. Some are even actively harmful which makes the decision to disable them that much easier. I'm not sure about the value of the signed/unsigned warnings. In theory they are useful but in practice they seem to just force us to add casts.
,
Jun 7 2018
> roughly consistent between compilers We only build with one compiler now. Do you mean between that one compiler between platforms, or...?
,
Jun 9 2018
,
Jun 11 2018
Yes, we only build with one compiler, except that there are occasional times when it is helpful to build with VC++. The extra warnings that VC++ gives are offering no value (because we don't formally build with VC++) while adding some cost (because we still informally build with VC++) so harmonizing (i.e.; disabling some VC++ warnings) is very tempting. Basically, in the cost/benefit analysis for VC++ warnings the benefit has dropped.
,
Jun 12 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0a8928dc97b033053a86ac6ae7471b1e9aaf319c commit 0a8928dc97b033053a86ac6ae7471b1e9aaf319c Author: Robert Liao <robliao@chromium.org> Date: Tue Jun 12 00:17:07 2018 MSVC: Ignore Nonstandard Extension Warnings All of our toolchains need to support these extensions. BUG=848979 Change-Id: I84ee2768c437c04b3d6362ab14fe7990ac33dfa0 Reviewed-on: https://chromium-review.googlesource.com/1089204 Commit-Queue: Robert Liao <robliao@chromium.org> Reviewed-by: Nico Weber <thakis@chromium.org> Cr-Commit-Position: refs/heads/master@{#566222} [modify] https://crrev.com/0a8928dc97b033053a86ac6ae7471b1e9aaf319c/build/config/compiler/BUILD.gn
,
Jun 13 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b5ef312bcef013e58cb8a441d6e42f3a92fdfe29 commit b5ef312bcef013e58cb8a441d6e42f3a92fdfe29 Author: Robert Liao <robliao@chromium.org> Date: Wed Jun 13 20:39:12 2018 MSVC: Disable Many of the Remaining Warnings Not Included in the Clang Build. BUG=848979 Change-Id: I13beb6efc1dbc86541b639661f2550b519982eca Reviewed-on: https://chromium-review.googlesource.com/1099047 Reviewed-by: Nico Weber <thakis@chromium.org> Commit-Queue: Robert Liao <robliao@chromium.org> Cr-Commit-Position: refs/heads/master@{#566973} [modify] https://crrev.com/b5ef312bcef013e58cb8a441d6e42f3a92fdfe29/build/config/compiler/BUILD.gn
,
Oct 9
|
||||
►
Sign in to add a comment |
|||||||
Comment 1 by thakis@chromium.org
, Jun 2 2018