<div dir="ltr">(thanks for pointing me to this thread - sorry I missed it)<br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Mar 20, 2015 at 4:26 PM, Kaylor, Andrew <span dir="ltr"><<a href="mailto:andrew.kaylor@intel.com" target="_blank">andrew.kaylor@intel.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div lang="EN-US" link="blue" vlink="purple">
<div>
<p class="MsoNormal">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.<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">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.<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">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):<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">Warnings that should almost certainly be disabled<u></u><u></u></p>
<p class="MsoNormal">-------------------------------------------------------------<u></u><u></u></p>
<p class="MsoNormal">warning C4100: unreferenced formal parameter (263666 times)<u></u><u></u></p>
<p class="MsoNormal">warning C4127: conditional expression is constant (101120 times)<u></u><u></u></p>
<p class="MsoNormal">warning C4512: assignment operator could not be generated (40472 times)<u></u><u></u></p>
<p class="MsoNormal">warning C4505: unreferenced local function has been removed (6606 times)</p></div></div></blockquote><div><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple"><div><p class="MsoNormal"><u></u><u></u></p>
<p class="MsoNormal">warning C4610: [...] can never be instantiated (1714 times)</p></div></div></blockquote><div><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple"><div><p class="MsoNormal"><u></u><u></u></p>
<p class="MsoNormal">warning C4510: default constructor could not be generated (1714 times)<u></u><u></u></p>
<p class="MsoNormal">warning C4702: unreachable code (1343 times)</p></div></div></blockquote><div><br>Even Clang's improved -Wunreachable-code isn't on by default yet. Maybe one day - it needs more cleanup.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple"><div><p class="MsoNormal"><u></u><u></u></p>
<p class="MsoNormal">warning C4706: assignment within conditional expression (296 times)</p></div></div></blockquote><div><br>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)<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple"><div><p class="MsoNormal"><u></u><u></u></p>
<p class="MsoNormal"><u></u><br></p><p class="MsoNormal"><u></u></p>
<p class="MsoNormal">Warnings that seem like they should be fixed but will take a little time to correct<u></u><u></u></p>
<p class="MsoNormal">--------------------------------------------------------------------------------------------------<u></u><u></u></p>
<p class="MsoNormal">warning C4245: signed/unsigned mismatch (962 times)</p></div></div></blockquote><div><br>If we want to hold this bar, we should consider enabling Clang/GCC's -Wsign-compare?<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple"><div><p class="MsoNormal"><u></u><u></u></p>
<p class="MsoNormal">warning C4310: cast truncates constant value (216 times)</p></div></div></blockquote><div><br></div><div>Again, could be improvements to Clang -Wconstant-conversion to catch these.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple"><div><p class="MsoNormal"><u></u><u></u></p>
<p class="MsoNormal">warning C4701: potentially uninitialized local variable (123 times)<u></u><u></u></p>
<p class="MsoNormal">warning C4703: potentially uninitialized local pointer variable (40 times)</p></div></div></blockquote><div><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple"><div><p class="MsoNormal"><u></u><u></u></p>
<p class="MsoNormal">warning C4389: signed/unsigned mismatch (28 times)</p></div></div></blockquote><div><br>Wonder how this is different from C4245?<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple"><div><p class="MsoNormal"><u></u><u></u></p>
<p class="MsoNormal"><u></u><br></p><p class="MsoNormal"><u></u></p>
<p class="MsoNormal">Warnings that should probably be fixed<u></u><u></u></p>
<p class="MsoNormal">-------------------------------------------------<u></u><u></u></p>
<p class="MsoNormal">warning C4189: local variable is initialized but not referenced (6 times)</p></div></div></blockquote><div><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple"><div><p class="MsoNormal"><u></u><u></u></p>
<p class="MsoNormal">warning C4204: nonstandard extension used : non-constant aggregate initializer (4 times)</p></div></div></blockquote><div><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple"><div><p class="MsoNormal"><u></u><u></u></p>
<p class="MsoNormal">warning C4611: interaction between '_setjmp' and C++ object destruction is non-portable (2 times)</p></div></div></blockquote><div><br>Curious - again, worth comparing whatever false positives to whatever holes clang might have, etc.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple"><div><p class="MsoNormal"><u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">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).</p></div></div></blockquote><div><br>Perhaps?<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple"><div><p class="MsoNormal"><u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">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.<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">Does anyone see any problems with the way I have categorized these warnings?</p></div></div></blockquote><div><br>Just to summarize from the other thread:<br><br>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.<br><br><br><br>- David<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple"><div><p class="MsoNormal"><u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">Thanks,<u></u><u></u></p>
<p class="MsoNormal">Andy<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
</div>
<br>_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:LLVMdev@cs.uiuc.edu">LLVMdev@cs.uiuc.edu</a> <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br>
<br></blockquote></div><br></div></div>