<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 24, 2015 at 1:24 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><span class="">
<p class="MsoNormal">>If your goal is to avoid breaking the Clang -Werror build, I don't think there's much you can do other than use a Clang build. But honestly enough people don't use it that those of us who do are used to cleaning up what gets through, so
I wouldn't worry about it too much if I were you.<span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
</span><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">My goal is to be able to turn on the “all warnings” option for MSVC builds. I understand that this won’t exactly replicate the Clang warning set, but I’m working
from the premise that warnings should be fixed because it makes the code better. So even if a /W4 build with MSVC doesn’t report everything that Clang would report, I still think it will be helpful.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">I think that as a side effect it has the potential to make breaking the Clang -Werror less common, at least for me personally.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Anyway, if you don’t object, I’ll take out the disputed changes and commit the changes we’ve agreed on. We can see how this evolves from there.</span></p></div></div></blockquote><div><br>Sure, of course.<br><br>- Dave<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"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">-Andy<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> <a href="mailto:llvm-commits-bounces@cs.uiuc.edu" target="_blank">llvm-commits-bounces@cs.uiuc.edu</a> [mailto:<a href="mailto:llvm-commits-bounces@cs.uiuc.edu" target="_blank">llvm-commits-bounces@cs.uiuc.edu</a>]
<b>On Behalf Of </b>David Blaikie<br>
<b>Sent:</b> Tuesday, March 24, 2015 11:43 AM<br>
<b>To:</b> <a href="mailto:reviews%2BD8572%2Bpublic%2B921eaa0cb55f1dac@reviews.llvm.org" target="_blank">reviews+D8572+public+921eaa0cb55f1dac@reviews.llvm.org</a><br>
<b>Cc:</b> <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<b>Subject:</b> Re: [PATCH] Disable specific MSVC warnings so that W4 warnings can be enabled.<u></u><u></u></span></p><div><div class="h5">
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal">On Tue, Mar 24, 2015 at 11:29 AM, Andy Kaylor <<a href="mailto:andrew.kaylor@intel.com" target="_blank">andrew.kaylor@intel.com</a>> wrote:<u></u><u></u></p>
<p class="MsoNormal">REPOSITORY<br>
rL LLVM<br>
<br>
================<br>
Comment at: cmake/modules/HandleLLVMOptions.cmake:286<br>
@@ +285,3 @@<br>
+ -wd4706 # Suppress 'assignment within conditional expression'<br>
+ -wd4310 # Suppress 'cast truncates constant value'<br>
+ -wd4701 # Suppress 'potentially uninitialized local variable'<br>
----------------<br>
dblaikie wrote:<br>
> You mentioned this one fired ~200 times - this might actually be worth doing a small sample check to see what it's firing on. Perhaps these are things we should fix - unless they appear in dependent expressions in templates, in which case they're probably
false positives.<br>
><br>
> The rest of these disables I'm totally fine with - if you want, please commit them separately/ahead of the rest of the review.<br>
These look pretty harmless. All of the ones I looked at were something like these:<br>
<br>
```<br>
uint8_t(~0U)<br>
(char)0xFF<br>
(uint16_t)-1U<u></u><u></u></p>
<div>
<p class="MsoNormal"><br>
Yeah, those all look like they do exactly what the user intended... I wonder if MSVC expects those to be written some other way? *shrug*<br>
<u></u><u></u></p>
</div>
<blockquote style="border:none;border-left:solid #cccccc 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal">```<br>
<br>
================<br>
Comment at: lib/Support/APFloat.cpp:1433<br>
@@ -1432,3 +1432,3 @@<br>
an addition or subtraction. */<br>
- subtract ^= sign ^ rhs.sign;<br>
+ subtract ^= (sign ^ rhs.sign) != 0;<br>
<br>
----------------<br>
dblaikie wrote:<br>
> andrew.w.kaylor wrote:<br>
> > This addresses warning C4805: unsafe mix of type <X> and type <Y> in operation.<br>
> This one's been fixed (slightly differently) by Aaron Ballman. We had some more discussions about the same issues/principles in that review thread too - I think he's OK with the idea of disabling this warning.<br>
OK<br>
<br>
================<br>
Comment at: lib/Target/ARM/InstPrinter/ARMInstPrinter.cpp:641<br>
@@ -640,3 +640,3 @@<br>
unsigned Op = ARM_AM::getAM5Op(MO2.getImm());<br>
- if (AlwaysPrintImm0 || ImmOffs || Op == ARM_AM::sub) {<br>
+ if (ImmOffs || Op == ARM_AM::sub || AlwaysPrintImm0) {<br>
O << ", "<br>
----------------<br>
dblaikie wrote:<br>
> andrew.w.kaylor wrote:<br>
> > This addresses warning C4189: local variable is initialized but not referenced.<br>
> ><br>
> > AlwaysPrintImm0 is a template parameter that can short circuit the condition so that ImmOffs and Op are never referenced.<br>
> This seems like a step backwards, actually - we try to deliberately put the cheap tests first (and a compile time constant is the cheapest test). Granted any compiler worth its salt will kill the dead expressions, but there's no need to ask it to do that
work.<br>
><br>
> I'm fairly strongly in favor of disabling this warning rather than making this code change.<br>
If it were just cases like this covered by the warning I would definitely agree that it's preferable to disable the warning. The problem, as Aaron Ballman mentioned, is that this same warning reports the basic case.<br>
```<br>
void foo() {<br>
int i = 0;<br>
// Doesn't use i<br>
}<br>
```<br>
That's exactly the sort of problem that I want to see when compiling with MSVC that motivated me to undertake this change set.<u></u><u></u></p>
</blockquote>
<div>
<p class="MsoNormal"><br>
If your goal is to avoid breaking the Clang -Werror build, I don't think there's much you can do other than use a Clang build. But honestly enough people don't use it that those of us who do are used to cleaning up what gets through, so I wouldn't worry about
it too much if I were you.<br>
<u></u><u></u></p>
</div>
<blockquote style="border:none;border-left:solid #cccccc 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal">I hear what you're saying about the change here moving backward, though the way the code is written the compiler still has to know that the calls above the condition have no side effects to eliminate them. <u></u><u></u></p>
</blockquote>
<div>
<p class="MsoNormal"><br>
Yep.<br>
<u></u><u></u></p>
</div>
<blockquote style="border:none;border-left:solid #cccccc 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal">How about if instead I rewrite the condition like this?<br>
<br>
```<br>
if (AlwaysPrintImm0 ||<br>
ARM_AM::getAM5Offset(MO2.getImm()) ||<u></u><u></u></p>
</blockquote>
<div>
<p class="MsoNormal"><br>
Except that ImmOffs is used within the if block too... <br>
<u></u><u></u></p>
</div>
<blockquote style="border:none;border-left:solid #cccccc 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal"> ARM_AM::getAM5Op(MO2.getImm()) == ARM_AM::sub) {<br>
```<br>
<br>
================<br>
Comment at: tools/llvm-c-test/metadata.c:21<br>
@@ -21,1 +20,3 @@<br>
+ LLVMValueRef values[1];<br>
+ values[0] = LLVMConstInt(LLVMInt32Type(), 0, 0);<br>
<br>
----------------<br>
dblaikie wrote:<br>
> andrew.w.kaylor wrote:<br>
> > This addresses warning C4204: nonstandard extension used : non-constant aggregate initializer.<br>
> Clang has a warning for this (-Wc99-extensions) but we don't enable it, so I'd say it's reasonable to disable MSVC's equivalent for consistency. Unless someone wants to look at turning Clang's version on & seeing what breaks...<br>
I have no objection to disabling this warning.<u></u><u></u></p>
<div>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><br>
<a href="http://reviews.llvm.org/D8572" target="_blank">http://reviews.llvm.org/D8572</a><br>
<br>
EMAIL PREFERENCES<br>
<a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">
http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
<br>
<u></u><u></u></p>
</div>
</div>
</blockquote>
</div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
</div>
</div></div></div>
</div>
</blockquote></div><br></div></div>