v2fsm-listen-darwin: combine bit operations

> From: Seija Kijin <doremylover123@gmail.com>
> 
> Signed-off-by: Seija Kijin <doremylover123@gmail.com>
> ---
>      fsm-listen-darwin: combine bit operations
>      
>      Signed-off-by: Seija Kijin doremylover123@gmail.com
> 
> 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 <doremylover123@gmail.com>
>        
>         ## Commit message ##
>       -    fsm-listen-daarwin: combine bit operations
>       +    fsm-listen-darwin: combine bit operations
>        
>            Signed-off-by: Seija Kijin <doremylover123@gmail.com>
>        
> 
> 
>   compat/fsmonitor/fsm-listen-darwin.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/compat/fsmonitor/fsm-listen-darwin.c b/compat/fsmonitor/fsm-listen-darwin.c
> index 97a55a6f0a4..fccdd21d858 100644
> --- a/compat/fsmonitor/fsm-listen-darwin.c
> +++ b/compat/fsmonitor/fsm-listen-darwin.c
> @@ -129,9 +129,9 @@ static int ef_is_root_renamed(const FSEventStreamEventFlags ef)
>   
>   static int ef_is_dropped(const FSEventStreamEventFlags ef)
>   {
> -	return (ef & kFSEventStreamEventFlagMustScanSubDirs ||
> -		ef & kFSEventStreamEventFlagKernelDropped ||
> -		ef & kFSEventStreamEventFlagUserDropped);
> +	return (ef & (kFSEventStreamEventFlagMustScanSubDirs |
> +		      kFSEventStreamEventFlagKernelDropped |
> +		      kFSEventStreamEventFlagUserDropped));
>   }

>>     static int ef_is_dropped(const FSEventStreamEventFlags ef)
>>   {
>> -	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.

> Jeff Hostetler <git@jeffhostetler.com> writes:
> 
>>>      static int ef_is_dropped(const FSEventStreamEventFlags ef)
>>>    {
>>> -	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.
> 
> 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