mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <>
To: Jeff Hostetler <>
Cc: Rose via GitGitGadget <>,, Seija Kijin <>
Subject: Re: [PATCH v2] fsm-listen-darwin: combine bit operations
Date: Fri, 20 Jan 2023 09:52:38 -0800	[thread overview]
Message-ID: <xmqqwn5hw0t5.fsf@gitster.g> (raw)
In-Reply-To: <> (Jeff Hostetler's message of "Fri, 20 Jan 2023 10:48:51 -0500")

Jeff Hostetler <> 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 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
two(unsigned int):
        xor     eax, eax
        and     edi, 7
        setne   al

  reply	other threads:[~2023-01-20 17:53 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-17 21:25 [PATCH] fsm-listen-daarwin: combine bit operations Rose via GitGitGadget
2023-01-17 21:54 ` [PATCH v2] fsm-listen-darwin: " Rose via GitGitGadget
2023-01-20 15:48   ` Jeff Hostetler
2023-01-20 17:52     ` Junio C Hamano [this message]
2023-01-20 19:48       ` Jeff Hostetler

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqwn5hw0t5.fsf@gitster.g \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).