{"thread":{"id":"59111","subject":"[PATCH v2] fsm-listen-darwin: combine bit operations","startedAt":"2023-01-17T22:41:26Z","lastAt":"2023-01-20T19:49:17Z","messageCount":5,"participants":["Rose via GitGitGadget","Jeff Hostetler","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"470550","messageId":"pull.1437.v2.git.git.1673992448371.gitgitgadget@gmail.com","threadId":"59111","inReplyTo":"pull.1437.git.git.1673990756466.gitgitgadget@gmail.com","subject":"[PATCH v2] fsm-listen-darwin: combine bit operations","fromName":"Rose via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-17T21:54:08Z","receivedAt":"2023-01-17T22:41:26Z","isPatch":true,"sender":{"key":"ckelsch@jgrcpa.com","avatar":null},"body":"From: Seija Kijin \n\nSigned-off-by: Seija Kijin \n---\n fsm-listen-darwin: combine bit operations\n \n Signed-off-by: Seija Kijin doremylover123@gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1437%2FAtariDreams%2Fdarwin-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1437/AtariDreams/darwin-v2\nPull-Request: https://github.com/git/git/pull/1437\n\nRange-diff vs v1:\n\n 1: a98654c7507 ! 1: 9943d52654f fsm-listen-daarwin: combine bit operations\n @@ Metadata\n Author: Seija Kijin \n \n ## Commit message ##\n - fsm-listen-daarwin: combine bit operations\n + fsm-listen-darwin: combine bit operations\n \n Signed-off-by: Seija Kijin \n \n\n\n compat/fsmonitor/fsm-listen-darwin.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/compat/fsmonitor/fsm-listen-darwin.c b/compat/fsmonitor/fsm-listen-darwin.c\nindex 97a55a6f0a4..fccdd21d858 100644\n--- a/compat/fsmonitor/fsm-listen-darwin.c\n+++ b/compat/fsmonitor/fsm-listen-darwin.c\n@@ -129,9 +129,9 @@ static int ef_is_root_renamed(const FSEventStreamEventFlags ef)\n \n static int ef_is_dropped(const FSEventStreamEventFlags ef)\n {\n-\treturn (ef & kFSEventStreamEventFlagMustScanSubDirs ||\n-\t\tef & kFSEventStreamEventFlagKernelDropped ||\n-\t\tef & kFSEventStreamEventFlagUserDropped);\n+\treturn (ef & (kFSEventStreamEventFlagMustScanSubDirs |\n+\t\t kFSEventStreamEventFlagKernelDropped |\n+\t\t kFSEventStreamEventFlagUserDropped));\n }\n \n /*\n\nbase-commit: a7caae2729742fc80147bca1c02ae848cb55921a\n-- \ngitgitgadget\n"},{"id":"470564","messageId":"pull.1437.git.git.1673990756466.gitgitgadget@gmail.com","threadId":"59111","inReplyTo":null,"subject":"[PATCH] fsm-listen-daarwin: combine bit operations","fromName":"Rose via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-17T21:25:56Z","receivedAt":"2023-01-17T23:35:59Z","isPatch":true,"sender":{"key":"ckelsch@jgrcpa.com","avatar":null},"body":"From: Seija Kijin \n\nSigned-off-by: Seija Kijin \n---\n fsm-listen-daarwin: combine bit operations\n \n Signed-off-by: Seija Kijin doremylover123@gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1437%2FAtariDreams%2Fdarwin-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1437/AtariDreams/darwin-v1\nPull-Request: https://github.com/git/git/pull/1437\n\n compat/fsmonitor/fsm-listen-darwin.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/compat/fsmonitor/fsm-listen-darwin.c b/compat/fsmonitor/fsm-listen-darwin.c\nindex 97a55a6f0a4..fccdd21d858 100644\n--- a/compat/fsmonitor/fsm-listen-darwin.c\n+++ b/compat/fsmonitor/fsm-listen-darwin.c\n@@ -129,9 +129,9 @@ static int ef_is_root_renamed(const FSEventStreamEventFlags ef)\n \n static int ef_is_dropped(const FSEventStreamEventFlags ef)\n {\n-\treturn (ef & kFSEventStreamEventFlagMustScanSubDirs ||\n-\t\tef & kFSEventStreamEventFlagKernelDropped ||\n-\t\tef & kFSEventStreamEventFlagUserDropped);\n+\treturn (ef & (kFSEventStreamEventFlagMustScanSubDirs |\n+\t\t kFSEventStreamEventFlagKernelDropped |\n+\t\t kFSEventStreamEventFlagUserDropped));\n }\n \n /*\n\nbase-commit: a7caae2729742fc80147bca1c02ae848cb55921a\n-- \ngitgitgadget\n"},{"id":"470791","messageId":"021ab1ab-b90a-5a24-23c4-44e46d87c476@jeffhostetler.com","threadId":"59111","inReplyTo":"pull.1437.v2.git.git.1673992448371.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] fsm-listen-darwin: combine bit operations","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2023-01-20T15:48:51Z","receivedAt":"2023-01-20T15:49:00Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 1/17/23 4:54 PM, Rose via GitGitGadget wrote:\n> From: Seija Kijin \n> \n> Signed-off-by: Seija Kijin \n> ---\n> fsm-listen-darwin: combine bit operations\n> \n> Signed-off-by: Seija Kijin doremylover123@gmail.com\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1437%2FAtariDreams%2Fdarwin-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1437/AtariDreams/darwin-v2\n> Pull-Request: https://github.com/git/git/pull/1437\n> \n> Range-diff vs v1:\n> \n> 1: a98654c7507 ! 1: 9943d52654f fsm-listen-daarwin: combine bit operations\n> @@ Metadata\n> Author: Seija Kijin \n> \n> ## Commit message ##\n> - fsm-listen-daarwin: combine bit operations\n> + fsm-listen-darwin: combine bit operations\n> \n> Signed-off-by: Seija Kijin \n> \n> \n> \n> compat/fsmonitor/fsm-listen-darwin.c | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n> \n> diff --git a/compat/fsmonitor/fsm-listen-darwin.c b/compat/fsmonitor/fsm-listen-darwin.c\n> index 97a55a6f0a4..fccdd21d858 100644\n> --- a/compat/fsmonitor/fsm-listen-darwin.c\n> +++ b/compat/fsmonitor/fsm-listen-darwin.c\n> @@ -129,9 +129,9 @@ static int ef_is_root_renamed(const FSEventStreamEventFlags ef)\n> \n> static int ef_is_dropped(const FSEventStreamEventFlags ef)\n> {\n> -\treturn (ef & kFSEventStreamEventFlagMustScanSubDirs ||\n> -\t\tef & kFSEventStreamEventFlagKernelDropped ||\n> -\t\tef & kFSEventStreamEventFlagUserDropped);\n> +\treturn (ef & (kFSEventStreamEventFlagMustScanSubDirs |\n> +\t\t kFSEventStreamEventFlagKernelDropped |\n> +\t\t kFSEventStreamEventFlagUserDropped));\n> }\n\nTechnically, the returned value is slightly different, but\nthe only caller is just checking for non-zero, so it doesn't\nmatter.\n\nSo this is fine.\n\nThanks,\nJeff\n\n"},{"id":"470798","messageId":"xmqqwn5hw0t5.fsf@gitster.g","threadId":"59111","inReplyTo":"021ab1ab-b90a-5a24-23c4-44e46d87c476@jeffhostetler.com","subject":"Re: [PATCH v2] fsm-listen-darwin: combine bit operations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-20T17:52:38Z","receivedAt":"2023-01-20T17:53:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff Hostetler writes:\n\n>> static int ef_is_dropped(const FSEventStreamEventFlags ef)\n>> {\n>> -\treturn (ef & kFSEventStreamEventFlagMustScanSubDirs ||\n>> -\t\tef & kFSEventStreamEventFlagKernelDropped ||\n>> -\t\tef & kFSEventStreamEventFlagUserDropped);\n>> +\treturn (ef & (kFSEventStreamEventFlagMustScanSubDirs |\n>> +\t\t kFSEventStreamEventFlagKernelDropped |\n>> +\t\t kFSEventStreamEventFlagUserDropped));\n>> }\n>\n> Technically, the returned value is slightly different, but\n> the only caller is just checking for non-zero, so it doesn't\n> matter.\n>\n> So this is fine.\n\nBut is it worth the code churn and reviewer bandwidth? Don't we\nhave better things to spend our time on?\n\nI would not be surprised if a smart enough compiler used the same\ntransformartion as this patch does manually as an optimization.\n\nThen it matters more which one of the two is more readable by our\ndevelopers. And the original matches how we humans would think, I\nwould imagine. ef might have MustScanSubdirs bit, KernelDropped\nbit, or UserDropped bit and in these cases we want to say that ef is\ndropped. Arguably, the original is more readble, and it would be a\ngood change to adopt if there is an upside, like the updated code\nresulting in markedly more efficient binary.\n\nSo, this might be technically fine, but I am not enthused to see\nthese kind of code churning patches with dubious upside. An\noptimization patch should be able to demonstrate its benefit with a\nsolid benchmark, or at least a clear difference in generated code.\n\nIn fact.\n\nCompiler explorer godbolt.org tells me that gcc 12 with -O2 compiles\nthe following two functions into identical assembly. The !! prefix\nused in the second example is different from the postimage of what\nSeija posted, but this being a file-scope static function, I would\nexpect the compiler to notice that the actual value would not matter\nto the callers, only the truth value, does.\n\n\n* Input *\nint one(unsigned int num) {\n return ((num & 01) ||\n (num & 02) || (num & 04));\n}\n\nint two(unsigned int num) {\n return !!((num) & (01|02|04));\n}\n\n* Assembly *\none(unsigned int):\n xor eax, eax\n and edi, 7\n setne al\n ret\ntwo(unsigned int):\n xor eax, eax\n and edi, 7\n setne al\n ret\n"},{"id":"470801","messageId":"d9873e9b-6225-169b-4829-92f069b943af@jeffhostetler.com","threadId":"59111","inReplyTo":"xmqqwn5hw0t5.fsf@gitster.g","subject":"Re: [PATCH v2] fsm-listen-darwin: combine bit operations","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2023-01-20T19:48:58Z","receivedAt":"2023-01-20T19:49:17Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 1/20/23 12:52 PM, Junio C Hamano wrote:\n> Jeff Hostetler writes:\n> \n>>> static int ef_is_dropped(const FSEventStreamEventFlags ef)\n>>> {\n>>> -\treturn (ef & kFSEventStreamEventFlagMustScanSubDirs ||\n>>> -\t\tef & kFSEventStreamEventFlagKernelDropped ||\n>>> -\t\tef & kFSEventStreamEventFlagUserDropped);\n>>> +\treturn (ef & (kFSEventStreamEventFlagMustScanSubDirs |\n>>> +\t\t kFSEventStreamEventFlagKernelDropped |\n>>> +\t\t kFSEventStreamEventFlagUserDropped));\n>>> }\n>>\n>> Technically, the returned value is slightly different, but\n>> the only caller is just checking for non-zero, so it doesn't\n>> matter.\n>>\n>> So this is fine.\n> \n> But is it worth the code churn and reviewer bandwidth? Don't we\n> have better things to spend our time on?\n> \n> I would not be surprised if a smart enough compiler used the same\n> transformartion as this patch does manually as an optimization.\n> \n> Then it matters more which one of the two is more readable by our\n> developers. And the original matches how we humans would think, I\n> would imagine. ef might have MustScanSubdirs bit, KernelDropped\n> bit, or UserDropped bit and in these cases we want to say that ef is\n> dropped. Arguably, the original is more readble, and it would be a\n> good change to adopt if there is an upside, like the updated code\n> resulting in markedly more efficient binary.\n> \n> So, this might be technically fine, but I am not enthused to see\n> these kind of code churning patches with dubious upside. An\n> optimization patch should be able to demonstrate its benefit with a\n> solid benchmark, or at least a clear difference in generated code.\n> \n> In fact.\n> \n> Compiler explorer godbolt.org tells me that gcc 12 with -O2 compiles\n> the following two functions into identical assembly. The !! prefix\n> used in the second example is different from the postimage of what\n> Seija posted, but this being a file-scope static function, I would\n> expect the compiler to notice that the actual value would not matter\n> to the callers, only the truth value, does.\n> \n> \n> * Input *\n> int one(unsigned int num) {\n> return ((num & 01) ||\n> (num & 02) || (num & 04));\n> }\n> \n> int two(unsigned int num) {\n> return !!((num) & (01|02|04));\n> }\n> \n> * Assembly *\n> one(unsigned int):\n> xor eax, eax\n> and edi, 7\n> setne al\n> ret\n> two(unsigned int):\n> xor eax, eax\n> and edi, 7\n> setne al\n> ret\n\nagreed. i didn't think the change was really worth the bother\nand churn. personally, i prefer the conceptual clarity of the\ncode the way I wrote it.\n\nand i was wondering if the compiler would generate the same\nresult, but didn't take the time (read: was too lazy) to actually\nverify that.\n\nall i was intending to say was that it wasn't a wrong change.\n\njeff\n"}]}