[LLVMdev] Enabling stricter warnings for Windows builds

Tue Apr 7 04:00:10 PDT 2015

It would great to report the defective warnings to the VC folks so they fix
them.

On Monday, March 23, 2015, David Blaikie <dblaikie at gmail.com> wrote:

> (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
> <javascript:_e(%7B%7D,'cvml','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 <javascript:_e(%7B%7D,'cvml','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/20150407/9e567c79/attachment.html>