<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Mar 23, 2015 at 4:01 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"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">I’ll try adding an explicit “!= 0” for this case. That should make the warning go away, in which case I’d be in favor of that.</span></p></div></div></blockquote><div><br>Mostly I'm concerned about adding things like this to fix warnings we'll just likely regress again because most of us develop with clang.<br><br>My rough thinking: If it's not worth adding to clang, it's not worth holding the codebase to & we should just disable the 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"><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> </span><span style="color:rgb(31,73,125);font-family:Calibri,sans-serif;font-size:11pt">I’ve got a handful of other small changes to fix warnings, so I can add this to the changes in my working set.</span></p></div></div></blockquote><div><br>Similarly for any of these.<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">-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"><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""> David Blaikie [mailto:<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>]
<br>
<b>Sent:</b> Monday, March 23, 2015 3:59 PM</span></p><div><div class="h5"><br>
<b>To:</b> Kaylor, Andrew<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: [llvm] r233004 - Refactor: Simplify boolean expressions in llvm Support<u></u><u></u></div></div><p></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 Mon, Mar 23, 2015 at 3:55 PM, Kaylor, Andrew <<a href="mailto:andrew.kaylor@intel.com" target="_blank">andrew.kaylor@intel.com</a>> wrote:<u></u><u></u></p>
<div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">I wasn’t sure what to think of this one. I guess it comes down to what the C++ standard says should
be happening here.</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">At first glance, a bitwise XOR of a bool with something else seems sketchy. On the other hand, if
the standard promises to convert the rhs to bool before the ^= operation then the warning shouldn’t be there.</span><u></u><u></u></p>
</div>
</div>
<div>
<p class="MsoNormal"><br>
Yeah, while doing Richard Thomson's cleanups I did hit one other case in clang that was something like:<br>
<br>
x |= a & b;<br>
<br>
where 'x' was boolean. Richard Smith suggested rephrasing it anyway, so it came out as:<br>
<br>
if (a & b)<br>
x = true;<br>
<br>
As I mentioned in the commit message, i'm open to adding the explicit != 0, etc, or something like that if people think it's particularly helpful here<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">
<div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">The C4805 warning seems to always apply to mixing bool with integer types. It didn’t show up anywhere
else in the llvm code base, so it doesn’t seem like disabling it across the board should be necessary.</span><u></u><u></u></p>
</div>
</div>
</blockquote>
<div>
<p class="MsoNormal"><br>
Well, my usual approach is either it's worth fixing or disabling so someone else doesn't trip over it and/or have to rehash the same conversation later, but... eh?<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">
<div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">-Andy</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"> </span><u></u><u></u></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""> David Blaikie [mailto:<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>]
<br>
<b>Sent:</b> Monday, March 23, 2015 3:47 PM<br>
<b>To:</b> Kaylor, Andrew<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: [llvm] r233004 - Refactor: Simplify boolean expressions in llvm Support</span><u></u><u></u></p>
<div>
<div>
<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 Mon, Mar 23, 2015 at 3:44 PM, Kaylor, Andrew <<a href="mailto:andrew.kaylor@intel.com" target="_blank">andrew.kaylor@intel.com</a>> wrote:<u></u><u></u></p>
<p class="MsoNormal">When I compile this with Visual Studio at /W4 I get this warning:<br>
<br>
llvm\lib\Support\APFloat.cpp(1433): warning C4805: '^=' : unsafe mix of type 'bool' and type 'unsigned int' in operation<u></u><u></u></p>
<div>
<p class="MsoNormal"><br>
Is this worth warning on (Clang doesn't, for example) - should we just disable the MSVC warning, then?<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-top:5.0pt;margin-right:0in;margin-bottom:5.0pt">
<p class="MsoNormal"><br>
According to the documentation this is a level 1 warning, so it probably appears with all builds. I just happen to have been working on a change to enable W4 warnings on Windows.<br>
<br>
-Andy<u></u><u></u></p>
<div>
<div>
<p class="MsoNormal"><br>
<br>
-----Original Message-----<br>
From: <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>] On Behalf Of David Blaikie<br>
Sent: Monday, March 23, 2015 12:46 PM<br>
To: <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
Subject: [llvm] r233004 - Refactor: Simplify boolean expressions in llvm Support<br>
<br>
Author: dblaikie<br>
Date: Mon Mar 23 14:45:40 2015<br>
New Revision: 233004<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=233004&view=rev" target="_blank">
http://llvm.org/viewvc/llvm-project?rev=233004&view=rev</a><br>
Log:<br>
Refactor: Simplify boolean expressions in llvm Support<br>
<br>
Simplify boolean expressions using `true` and `false` with `clang-tidy`<br>
<br>
Patch by Richard Thomson - I dropped the parens and != 0 test, for consistency with other patches/tests like this, but I'm open to the notion that we should add the explicit non-zero test in all these sort of cases (non-bool assigned to a bool).<br>
<br>
Differential Revision: <a href="http://reviews.llvm.org/D8526" target="_blank">http://reviews.llvm.org/D8526</a><br>
<br>
Modified:<br>
llvm/trunk/lib/Support/APFloat.cpp<br>
<br>
Modified: llvm/trunk/lib/Support/APFloat.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/APFloat.cpp?rev=233004&r1=233003&r2=233004&view=diff" target="_blank">
http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/APFloat.cpp?rev=233004&r1=233003&r2=233004&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Support/APFloat.cpp (original)<br>
+++ llvm/trunk/lib/Support/APFloat.cpp Mon Mar 23 14:45:40 2015<br>
@@ -1430,7 +1430,7 @@ APFloat::addOrSubtractSignificand(const<br>
<br>
/* Determine if the operation on the absolute values is effectively<br>
an addition or subtraction. */<br>
- subtract ^= (sign ^ rhs.sign) ? true : false;<br>
+ subtract ^= sign ^ rhs.sign;<br>
<br>
/* Are we bigger exponent-wise than the RHS? */<br>
bits = exponent - rhs.exponent;<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><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>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
</div>
</div></div></div>
</div>
</blockquote></div><br></div></div>