[LLVMdev] Enabling stricter warnings for Windows builds

[David Blaikie]

(thanks for pointing me to this thread - sorry I missed it)

On Fri, Mar 20, 2015 at 4:26 PM, Kaylor, Andrew <andrew.kaylor at intel.com>
wrote:

>  I’ve been guilty several times recently of committing code that
> introduced build warnings.  It’s a poor excuse, but my excuse is that I’ve
> been working on code for Windows and LLVM doesn’t enable strict warnings by
> default on Windows and produces nearly half a million warnings (literally)
> if you manually turn them on.  As such, I thought I’d make an effort to see
> what it would take to clean things up on Windows.
>
>
>
> Looking just at the main llvm project I found that despite the multitude
> of warnings, there were only 16 unique warnings being produced.  I see in
> HandleLLVMOptions.cmake that there are already 17 warnings being explicitly
> disabled, so I thought it would be reasonable to start by adding most of
> the warnings that are currently being produced to that list.
>
>
>
> I triaged the warnings that I was seeing according to how certain I was
> that disabling them was a reasonable thing to do.  Here’s what I came up
> with (with a glaring bias toward just disabling common warnings and fixing
> uncommon ones):
>
>
>
>
>
> Warnings that should almost certainly be disabled
>
> -------------------------------------------------------------
>
> warning C4100: unreferenced formal parameter (263666 times)
>
> warning C4127: conditional expression is constant (101120 times)
>
> warning C4512: assignment operator could not be generated (40472 times)
>
> warning C4505: unreferenced local function has been removed (6606 times)
>

This ^ is curious - I guess there's some misfeature in MSVC's
implementation of this warning? Clang & GCC have a -Wunused-function which
we have turned on and LLVM/Clang/etc build clean of this warning.


>  warning C4610: [...] can never be instantiated (1714 times)
>

Curious (again, given the high number of instances, I assume the warning is
overlooking something) - wonder what the deal is there. But just mild
curiosity, nothing important.


>  warning C4510: default constructor could not be generated (1714 times)
>
> warning C4702: unreachable code (1343 times)
>

Even Clang's improved -Wunreachable-code isn't on by default yet. Maybe one
day - it needs more cleanup.


>  warning C4706: assignment within conditional expression (296 times)
>

Only 300? I was going to say that I assume this warning doesn't support the
double-paren suppression that GCC/Clang's -Wparentheses dictate, but with
such a low hit rate I wonder if it's some other sub-set of cases MSVC is
seeing here. (or maybe we don't use even parentheses-bound assignment in
conditionals very often at all)


>
>  Warnings that seem like they should be fixed but will take a little time
> to correct
>
>
> --------------------------------------------------------------------------------------------------
>
> warning C4245: signed/unsigned mismatch (962 times)
>

If we want to hold this bar, we should consider enabling Clang/GCC's
-Wsign-compare?


>  warning C4310: cast truncates constant value (216 times)
>

Again, could be improvements to Clang -Wconstant-conversion to catch these.


>  warning C4701: potentially uninitialized local variable (123 times)
>
> warning C4703: potentially uninitialized local pointer variable (40 times)
>

These two are /probably/ not worth it (but worth checking a sample to see)
- Clang's -Wsometimes-uninitialized is designed to have a low/zero false
positive rate while diagnosing cases like this.


>  warning C4389: signed/unsigned mismatch (28 times)
>

Wonder how this is different from C4245?


>
>  Warnings that should probably be fixed
>
> -------------------------------------------------
>
> warning C4189: local variable is initialized but not referenced (6 times)
>

Clang diagnoses cases like this already - I wonder what subset MSVC is
catching that Clang is not. If it's types with non-trivial
construction/destruction, then this seems like a warning we might not want
to enable - it might false-positive on scoped devices, etc.


>  warning C4204: nonstandard extension used : non-constant aggregate
> initializer (4 times)
>

If this is a nonstandard extension we want to avoid then maybe breaking it
out of Clang's -Wpedantic (assuming it's there) into a separate flag
(assuming it's not already under a separate flag) & enabling both MSVC and
Clang's would be fine.


>  warning C4611: interaction between '_setjmp' and C++ object destruction
> is non-portable (2 times)
>

Curious - again, worth comparing whatever false positives to whatever holes
clang might have, etc.


>
>
>
>
> As a caveat, I didn’t look at the code for any of these warnings -- not
> one -- so I don’t know anything about how benign any of them might be.  I
> also don’t know why there are two different “signed/unsigned mismatch”
> warnings (though I’m guessing one is assignments and the other is
> comparisons).
>

Perhaps?


>
>
> I have a local build going right now with the first two groups of warnings
> disabled.  If it goes as expected, I’d like to commit this change along
> with fixes for the very small last group.  Then, after a reasonable
> adjustment period to let subprojects see how this looks, I’d like to turn
> on the “all warnings” option by default for Windows builds.
>
>
>
> Does anyone see any problems with the way I have categorized these
> warnings?
>

Just to summarize from the other thread:

My understanding and preference from past discussions is essentially: If it
isn't a diagnostic that's sufficiently valuable/correct/etc to implement in
Clang, it's not worth holding the LLVM codebase to that standard and we
should just disable whatever warning it is. I think that's been the general
philosophy in the past, to the best of my understanding.



- David


>
>
> Thanks,
>
> Andy
>
>
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20150323/8d63c931/attachment.html>