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

[David Blaikie]

On Mon, Mar 23, 2015 at 4:01 PM, Kaylor, Andrew <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]
> *Sent:* Monday, March 23, 2015 3:59 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 3:55 PM, Kaylor, Andrew <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]
> *Sent:* Monday, March 23, 2015 3:47 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 3:44 PM, Kaylor, Andrew <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] On Behalf Of David Blaikie
> Sent: Monday, March 23, 2015 12:46 PM
> To: 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
> 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/07e517e6/attachment.html>