fsm-listen-darwin: combine bit operations #1437
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/submit |
|
Submitted as pull.1437.git.git.1673990756466.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
AtariDreams
changed the title
|
/submit |
|
Submitted as pull.1437.v2.git.git.1673992448371.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Jeff Hostetler wrote (reply to this): On 1/17/23 4:54 PM, Rose via GitGitGadget wrote:
> 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));
> }
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.
Thanks,
Jeff
|
|
User |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): 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 |
|
On the Git mailing list, Jeff Hostetler wrote (reply to this): On 1/20/23 12:52 PM, Junio C Hamano wrote:
> 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
agreed. i didn't think the change was really worth the bother
and churn. personally, i prefer the conceptual clarity of the
code the way I wrote it.
and i was wondering if the compiler would generate the same
result, but didn't take the time (read: was too lazy) to actually
verify that.
all i was intending to say was that it wasn't a wrong change.
jeff |
Signed-off-by: Seija Kijin <doremylover123@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Seija Kijin doremylover123@gmail.com
cc: Jeff Hostetler git@jeffhostetler.com