Re: [PATCH] maint: Use logical rather than bitwise operators on bools

[Pádraig Brady]

Paul Eggert wrote:
> Pádraig Brady <address@hidden> writes:
> 
>> As well as being distracting, bitwise operators are
>> not short circuiting and brittle with C89 where
>> bool can be an int, i.e. > 1.
> 
> I don't find them distracting; on the contrary, I find them a helpful
> reminder that the expressions are boolean (in the traditional
> mathematical sense) and so I don't need to worry about the
> complexities of short-circuit evaluation.
> 
> Also, the conditional branches generated by short-circuit operators
> can make code harder to debug because GDB starts jumping all over the
> place when I single step.
> 
> Furthermore, bitwise operators often generate shorter and faster code,
> particularly on modern processors where conditional branches are far
> more expensive than logical operations.  For example, consider these
> functions:
> 
> bool F (bool a, bool b, bool c, bool d) { return a && b && c && d; }
> bool G (bool a, bool b, bool c, bool d) { return a & b & c & d; }

Well I did actually compare the assembly for chcon.c which
was the first I noticed, before I decided it might be worth doing:

$ objdump -S chcon.o > chcon.logical #and likewise for bitwise
$ sdiff chcon.bitwise chcon.logical
...
if (recurse & preserve_root)                                |   if (recurse && 
preserve_root)
   6c0:       0f b6 05 11 00 00 00    movzbl 0x11,%eax      |      6c0:       
80 3d 11 00 00 00 00    cmpb   $0x0,0x11
   6c7:       0f b6 4c 24 34          movzbl 0x34(%esp),%ec |      6c7:       
74 0b                   je     6d4 <main+0x3c
   6cc:       85 c8                   test   %ecx,%eax      |      6c9:       
80 7c 24 34 00          cmpb   $0x0,0x34(%esp
   6ce:       0f 85 8b 07 00 00       jne    e5f <main+0xb4 |      6ce:       
0f 85 3e 0a 00 00       jne    1112 <main+0xe
      error (EXIT_FAILURE, errno, _("failed to get attribut           error 
(EXIT_FAILURE, errno, _("failed to get attribut

You're right about the extra branch but for this common case at least
the size was the same. I did also check the speed with a test program
(attached) which showed the logical test was a bit faster on my pentium-m.
Note I compiled without optimization like `gcc -march=pentium-m chcon-test.c`
so that the loop was actually run, and I got these (averaged) results:

$ time ./bitwise
real    0m4.550s
$ time ./logical
real    0m4.173s

> For F, gcc -O2 (x86) currently generates 3 conditional branches
> (ouch!); for G it generates straightline code (good with pipelining).
> F is also quite a bit bigger than G (20 instructions vs 12; and 49
> bytes versus 30), and this size increase bloats the code and causes
> more instruction cache misses.
> 
> Bitwise operators are equivalent in behavior to short-circuit
> operations on boolean expressions (i.e., expressions involving just 0
> and 1) that do not have side effects or conditionally-undefined
> behavior. This is true regardless of whether the boolean type is
> implemented via 'int'.  Let's just leave them alone.

Well lots (most?) of the code already uses logical operators
and the mixing of both was confusing me a bit (I'm easily confused :))
I also noticed in expr.c that the null() function was not
short circuited which I changed in the hunk below.
As well as this probably being a lower performance, I needed
to examine the function to know if it had side effects which
one can discount when logical operators are used.
I'm worried about stuff like that creeping in if bitwise
operators are left in place.

diff --git a/src/expr.c b/src/expr.c
index aab09a1..88e0b47 100644
--- a/src/expr.c
+++ b/src/expr.c
@@ -923,7 +923,7 @@ eval1 (bool evaluate)
     {
       if (nextarg ("&"))
         {
-          r = eval2 (evaluate & ~ null (l));
+          r = eval2 (evaluate && !null (l));
           if (null (l) || null (r))
             {
               freev (l);
@@ -954,7 +954,7 @@ eval (bool evaluate)
     {
       if (nextarg ("|"))
         {
-          r = eval1 (evaluate & null (l));
+          r = eval1 (evaluate && null (l));
           if (null (l))
             {
               freev (l);

cheers,
Pádraig.

#include <stdbool.h>
#include <stdio.h>

int main(int argc, char** argv)
{
    bool recurse=false;
    bool preserve_root=true;
    int i;
    for (i=0; i<1000000000; i++)
      if (recurse && preserve_root)
          puts("true");
}