fsm-listen-daarwin: combine bit operations

From: Seija Kijin <[email protected]>

Signed-off-by Seija Kijin <[email protected]>

---
fsm-listen-darwin: combine bit operations

Signed-off-by Seija Kijin [email protected]

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1437%2FAtariDreams%2Fdarwin-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1437/AtariDreams/darwin-v2
Pull-Request: https://github.com/git/git/pull/1437

Range-diff vs v1:

1: a98654c7507 ! 1: 9943d52654f fsm-listen-daarwin: combine bit operations
@@ Metadata
Author: Seija Kijin <[email protected]>

## Commit message ##
- fsm-listen-daarwin: combine bit operations
+ fsm-listen-darwin: combine bit operations

Signed-off-by Seija Kijin <[email protected]>

  compat/fsmonitor/fsm-listen-darwin.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

compat/fsmonitor/fsm-listen-darwin.c +0 -0

index 97a55a6f0a4..fccdd21d858 100644

compat/fsmonitor/fsm-listen-darwin.c static int ef_is_root_renamed(const FSEventStreamEventFlags ef) · line 135

Quoted 9 lines

static int ef_is_dropped(const FSEventStreamEventFlags ef)
{

compat/fsmonitor/fsm-listen-darwin.c +3 -3

-	return (ef & kFSEventStreamEventFlagMustScanSubDirs ||
-		ef & kFSEventStreamEventFlagKernelDropped ||
-		ef & kFSEventStreamEventFlagUserDropped);
+	return (ef & (kFSEventStreamEventFlagMustScanSubDirs |
+		      kFSEventStreamEventFlagKernelDropped |
+		      kFSEventStreamEventFlagUserDropped));
  }

Technically, the returned value is slightly different, but the only caller is just checking for non-zero, so it doesn't matter.

So this is fine.

Quoted 14 lines · Jeff Hostetler

compat/fsmonitor/fsm-listen-darwin.c static int ef_is_root_renamed(const FSEventStreamEventFlags ef) · line 135

Quoted 9 lines

static int ef_is_dropped(const FSEventStreamEventFlags ef)
{

compat/fsmonitor/fsm-listen-darwin.c +3 -3

-	return (ef & kFSEventStreamEventFlagMustScanSubDirs ||
-		ef & kFSEventStreamEventFlagKernelDropped ||
-		ef & kFSEventStreamEventFlagUserDropped);
+	return (ef & (kFSEventStreamEventFlagMustScanSubDirs |
+		      kFSEventStreamEventFlagKernelDropped |
+		      kFSEventStreamEventFlagUserDropped));
   }

Technically, the returned value is slightly different, but the only caller is just checking for non-zero, so it doesn't matter.

Quoted 1 line

So this is fine.

But is it worth the code churn and reviewer bandwidth? Don't we have better things to spend our time on?

I would not be surprised if a smart enough compiler used the same transformartion as this patch does manually as an optimization.

Then it matters more which one of the two is more readable by our developers. And the original matches how we humans would think, I would imagine. ef might have MustScanSubdirs bit, KernelDropped bit, or UserDropped bit and in these cases we want to say that ef is dropped. Arguably, the original is more readble, and it would be a good change to adopt if there is an upside, like the updated code resulting in markedly more efficient binary.

So, this might be technically fine, but I am not enthused to see these kind of code churning patches with dubious upside. An optimization patch should be able to demonstrate its benefit with a solid benchmark, or at least a clear difference in generated code.

In fact.

Compiler explorer godbolt.org tells me that gcc 12 with -O2 compiles the following two functions into identical assembly. The !! prefix used in the second example is different from the postimage of what Seija posted, but this being a file-scope static function, I would expect the compiler to notice that the actual value would not matter to the callers, only the truth value, does.

* Input *

int one(unsigned int num) {
     return ((num & 01) ||
             (num & 02) || (num & 04));
}

int two(unsigned int num) {
     return !!((num) & (01|02|04));
}

* Assembly *
one(unsigned int):

         xor     eax, eax
         and     edi, 7
         setne   al
         ret
two(unsigned int):
         xor     eax, eax
         and     edi, 7
         setne   al

ret