[llvm] r233004 - Refactor: Simplify boolean expressions in llvm Support

[Kaylor, Andrew]

I see your point about only supporting warnings that clang might generate.

My intention is to turn on the MSVC warnings by default, which would give these warnings some visibility (assuming we have clean Windows buildbots), but I don’t mind aggressively disabling warnings that we don’t think are worth reporting.

I sent out an e-mail on Friday describing the few warnings I intended to do anything about.  I hope to send out a patch today for review of the changes I mean to make.

-Andy

From: David Blaikie [mailto:dblaikie at gmail.com]
Sent: Monday, March 23, 2015 4:10 PM
To: Kaylor, Andrew
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: [llvm] r233004 - Refactor: Simplify boolean expressions in llvm Support



On Mon, Mar 23, 2015 at 4:01 PM, Kaylor, Andrew <andrew.kaylor at intel.com<mailto:andrew.kaylor at intel.com>> wrote:
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.

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.

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.

 I’ve got a handful of other small changes to fix warnings, so I can add this to the changes in my working set.

Similarly for any of these.


-Andy

From: David Blaikie [mailto:dblaikie at gmail.com<mailto:dblaikie at gmail.com>]
Sent: Monday, March 23, 2015 3:59 PM

To: Kaylor, Andrew
Cc: llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
Subject: Re: [llvm] r233004 - Refactor: Simplify boolean expressions in llvm Support



On Mon, Mar 23, 2015 at 3:55 PM, Kaylor, Andrew <andrew.kaylor at intel.com<mailto:andrew.kaylor at intel.com>> wrote:
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.

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.

Yeah, while doing Richard Thomson's cleanups I did hit one other case in clang that was something like:

x |= a & b;

where 'x' was boolean. Richard Smith suggested rephrasing it anyway, so it came out as:

if (a & b)
  x = true;

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


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.

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?


-Andy


From: David Blaikie [mailto:dblaikie at gmail.com<mailto:dblaikie at gmail.com>]
Sent: Monday, March 23, 2015 3:47 PM
To: Kaylor, Andrew
Cc: llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
Subject: Re: [llvm] r233004 - Refactor: Simplify boolean expressions in llvm Support



On Mon, Mar 23, 2015 at 3:44 PM, Kaylor, Andrew <andrew.kaylor at intel.com<mailto:andrew.kaylor at intel.com>> wrote:
When I compile this with Visual Studio at /W4 I get this warning:

llvm\lib\Support\APFloat.cpp(1433): warning C4805: '^=' : unsafe mix of type 'bool' and type 'unsigned int' in operation

Is this worth warning on (Clang doesn't, for example) - should we just disable the MSVC warning, then?


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.

-Andy


-----Original Message-----
From: llvm-commits-bounces at cs.uiuc.edu<mailto:llvm-commits-bounces at cs.uiuc.edu> [mailto:llvm-commits-bounces at cs.uiuc.edu<mailto:llvm-commits-bounces at cs.uiuc.edu>] On Behalf Of David Blaikie
Sent: Monday, March 23, 2015 12:46 PM
To: llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
Subject: [llvm] r233004 - Refactor: Simplify boolean expressions in llvm Support

Author: dblaikie
Date: Mon Mar 23 14:45:40 2015
New Revision: 233004

URL: http://llvm.org/viewvc/llvm-project?rev=233004&view=rev
Log:
Refactor: Simplify boolean expressions in llvm Support

Simplify boolean expressions using `true` and `false` with `clang-tidy`

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).

Differential Revision: http://reviews.llvm.org/D8526

Modified:
    llvm/trunk/lib/Support/APFloat.cpp

Modified: llvm/trunk/lib/Support/APFloat.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/APFloat.cpp?rev=233004&r1=233003&r2=233004&view=diff
==============================================================================
--- llvm/trunk/lib/Support/APFloat.cpp (original)
+++ llvm/trunk/lib/Support/APFloat.cpp Mon Mar 23 14:45:40 2015
@@ -1430,7 +1430,7 @@ APFloat::addOrSubtractSignificand(const

   /* Determine if the operation on the absolute values is effectively
      an addition or subtraction.  */
-  subtract ^= (sign ^ rhs.sign) ? true : false;
+  subtract ^= sign ^ rhs.sign;

   /* Are we bigger exponent-wise than the RHS?  */
   bits = exponent - rhs.exponent;


_______________________________________________
llvm-commits mailing list
llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150323/c0912a55/attachment.html>