{"thread":{"id":"60608","subject":"[PATCH] Use ^=1 to toggle between 0 and 1","startedAt":"2023-12-12T17:17:51Z","lastAt":"2024-12-19T10:35:17Z","messageCount":26,"participants":["AtariDreams via GitGitGadget","Dragan Simic","Jeff King","René Scharfe","Junio C Hamano","Phillip Wood","phillip.wood123@gmail.com","AreaZR via GitGitGadget"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"485584","messageId":"pull.1620.git.git.1702401468082.gitgitgadget@gmail.com","threadId":"60608","inReplyTo":null,"subject":"[PATCH] Use ^=1 to toggle between 0 and 1","fromName":"AtariDreams via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-12-12T17:17:47Z","receivedAt":"2023-12-12T17:17:51Z","isPatch":true,"sender":{"key":"name:AtariDreams","avatar":null},"body":"From: Seija Kijin \n\nIf it is known that an int is either 1 or 0,\ndoing an exclusive or to switch instead of a\nmodulus makes more sense and is more efficient.\n\nSigned-off-by: Seija Kijin doremylover123@gmail.com\n---\n Use ^=1 to toggle between 0 and 1\n \n If it is known that an int is either 1 or 0, doing an exclusive or to\n switch instead of a modulus makes more sense and is more efficient.\n \n Signed-off-by: Seija Kijin doremylover123@gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1620%2FAtariDreams%2Fbuffer-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1620/AtariDreams/buffer-v1\nPull-Request: https://github.com/git/git/pull/1620\n\n builtin/fast-export.c | 4 ++--\n diff.c | 2 +-\n ident.c | 2 +-\n t/helper/test-path-utils.c | 2 +-\n 4 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 70aff515acb..f9f2c9dd850 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -593,8 +593,8 @@ static void anonymize_ident_line(const char **beg, const char **end)\n \tstruct ident_split split;\n \tconst char *end_of_header;\n \n-\tout = &buffers[which_buffer++];\n-\twhich_buffer %= ARRAY_SIZE(buffers);\n+\tout = &buffers[which_buffer];\n+\twhich_buffer ^= 1;\n \tstrbuf_reset(out);\n \n \t/* skip \"committer\", \"author\", \"tagger\", etc */\ndiff --git a/diff.c b/diff.c\nindex 2c602df10a3..91842b54753 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1191,7 +1191,7 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\t\t\t\t\t\t &pmb_nr);\n \n \t\t\tif (contiguous && pmb_nr && moved_symbol == l->s)\n-\t\t\t\tflipped_block = (flipped_block + 1) % 2;\n+\t\t\t\tflipped_block ^= 1;\n \t\t\telse\n \t\t\t\tflipped_block = 0;\n \ndiff --git a/ident.c b/ident.c\nindex cc7afdbf819..188826eed63 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -459,7 +459,7 @@ const char *fmt_ident(const char *name, const char *email,\n \tint want_name = !(flag & IDENT_NO_NAME);\n \n \tstruct strbuf *ident = &ident_pool[index];\n-\tindex = (index + 1) % ARRAY_SIZE(ident_pool);\n+\tindex ^= 1;\n \n \tif (!email) {\n \t\tif (whose_ident == WANT_AUTHOR_IDENT && git_author_email.len)\ndiff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c\nindex 70396fa3845..241136148a5 100644\n--- a/t/helper/test-path-utils.c\n+++ b/t/helper/test-path-utils.c\n@@ -185,7 +185,7 @@ static int check_dotfile(const char *x, const char **argv,\n \tint res = 0, expect = 1;\n \tfor (; *argv; argv++) {\n \t\tif (!strcmp(\"--not\", *argv))\n-\t\t\texpect = !expect;\n+\t\t\texpect ^= 1;\n \t\telse if (expect != (is_hfs(*argv) || is_ntfs(*argv)))\n \t\t\tres = error(\"'%s' is %s.git%s\", *argv,\n \t\t\t\t expect ? \"not \" : \"\", x);\n\nbase-commit: 1a87c842ece327d03d08096395969aca5e0a6996\n-- \ngitgitgadget\n"},{"id":"485585","messageId":"62c31371741b07ec5e012cb268294536@manjaro.org","threadId":"60608","inReplyTo":"pull.1620.git.git.1702401468082.gitgitgadget@gmail.com","subject":"Re: [PATCH] Use ^=1 to toggle between 0 and 1","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2023-12-12T17:29:05Z","receivedAt":"2023-12-12T17:29:08Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2023-12-12 18:17, AtariDreams via GitGitGadget wrote:\n> From: Seija Kijin \n> \n> If it is known that an int is either 1 or 0,\n> doing an exclusive or to switch instead of a\n> modulus makes more sense and is more efficient.\n\nQuite frankly, this doesn't seem like an improvement to me. It makes \nthe code much less readable, more error-prone, and may even end up \nproducing code that isn't portable.\n\nRegarding the efficiency, such optimizations may be perfectly fine as a \ntrade-off in some critical paths, but these cases don't seem like that.\n\n> Signed-off-by: Seija Kijin doremylover123@gmail.com\n> ---\n> Use ^=1 to toggle between 0 and 1\n> \n> If it is known that an int is either 1 or 0, doing an exclusive or \n> to\n> switch instead of a modulus makes more sense and is more efficient.\n> \n> Signed-off-by: Seija Kijin doremylover123@gmail.com\n> \n> Published-As:\n> https://github.com/gitgitgadget/git/releases/tag/pr-git-1620%2FAtariDreams%2Fbuffer-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git\n> pr-git-1620/AtariDreams/buffer-v1\n> Pull-Request: https://github.com/git/git/pull/1620\n> \n> builtin/fast-export.c | 4 ++--\n> diff.c | 2 +-\n> ident.c | 2 +-\n> t/helper/test-path-utils.c | 2 +-\n> 4 files changed, 5 insertions(+), 5 deletions(-)\n> \n> diff --git a/builtin/fast-export.c b/builtin/fast-export.c\n> index 70aff515acb..f9f2c9dd850 100644\n> --- a/builtin/fast-export.c\n> +++ b/builtin/fast-export.c\n> @@ -593,8 +593,8 @@ static void anonymize_ident_line(const char **beg,\n> const char **end)\n> \tstruct ident_split split;\n> \tconst char *end_of_header;\n> \n> -\tout = &buffers[which_buffer++];\n> -\twhich_buffer %= ARRAY_SIZE(buffers);\n> +\tout = &buffers[which_buffer];\n> +\twhich_buffer ^= 1;\n> \tstrbuf_reset(out);\n> \n> \t/* skip \"committer\", \"author\", \"tagger\", etc */\n> diff --git a/diff.c b/diff.c\n> index 2c602df10a3..91842b54753 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -1191,7 +1191,7 @@ static void mark_color_as_moved(struct \n> diff_options *o,\n> \t\t\t\t\t\t\t &pmb_nr);\n> \n> \t\t\tif (contiguous && pmb_nr && moved_symbol == l->s)\n> -\t\t\t\tflipped_block = (flipped_block + 1) % 2;\n> +\t\t\t\tflipped_block ^= 1;\n> \t\t\telse\n> \t\t\t\tflipped_block = 0;\n> \n> diff --git a/ident.c b/ident.c\n> index cc7afdbf819..188826eed63 100644\n> --- a/ident.c\n> +++ b/ident.c\n> @@ -459,7 +459,7 @@ const char *fmt_ident(const char *name, const char \n> *email,\n> \tint want_name = !(flag & IDENT_NO_NAME);\n> \n> \tstruct strbuf *ident = &ident_pool[index];\n> -\tindex = (index + 1) % ARRAY_SIZE(ident_pool);\n> +\tindex ^= 1;\n> \n> \tif (!email) {\n> \t\tif (whose_ident == WANT_AUTHOR_IDENT && git_author_email.len)\n> diff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c\n> index 70396fa3845..241136148a5 100644\n> --- a/t/helper/test-path-utils.c\n> +++ b/t/helper/test-path-utils.c\n> @@ -185,7 +185,7 @@ static int check_dotfile(const char *x, const char \n> **argv,\n> \tint res = 0, expect = 1;\n> \tfor (; *argv; argv++) {\n> \t\tif (!strcmp(\"--not\", *argv))\n> -\t\t\texpect = !expect;\n> +\t\t\texpect ^= 1;\n> \t\telse if (expect != (is_hfs(*argv) || is_ntfs(*argv)))\n> \t\t\tres = error(\"'%s' is %s.git%s\", *argv,\n> \t\t\t\t expect ? \"not \" : \"\", x);\n> \n> base-commit: 1a87c842ece327d03d08096395969aca5e0a6996\n"},{"id":"485590","messageId":"20231212200920.GC1127366@coredump.intra.peff.net","threadId":"60608","inReplyTo":"pull.1620.git.git.1702401468082.gitgitgadget@gmail.com","subject":"Re: [PATCH] Use ^=1 to toggle between 0 and 1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-12T20:09:20Z","receivedAt":"2023-12-12T20:09:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 12, 2023 at 05:17:47PM +0000, AtariDreams via GitGitGadget wrote:\n\n> diff --git a/builtin/fast-export.c b/builtin/fast-export.c\n> index 70aff515acb..f9f2c9dd850 100644\n> --- a/builtin/fast-export.c\n> +++ b/builtin/fast-export.c\n> @@ -593,8 +593,8 @@ static void anonymize_ident_line(const char **beg, const char **end)\n> \tstruct ident_split split;\n> \tconst char *end_of_header;\n> \n> -\tout = &buffers[which_buffer++];\n> -\twhich_buffer %= ARRAY_SIZE(buffers);\n> +\tout = &buffers[which_buffer];\n> +\twhich_buffer ^= 1;\n\nIn the current code, if the size of \"buffers\" is increased then\neverything would just work. But your proposed code (rather subtly) makes\nthe assumption that ARRAY_SIZE(buffers) is 2.\n\nSo even leaving aside questions of readability, I think the existing\ncode is much more maintainable.\n\n> diff --git a/diff.c b/diff.c\n> index 2c602df10a3..91842b54753 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -1191,7 +1191,7 @@ static void mark_color_as_moved(struct diff_options *o,\n> \t\t\t\t\t\t\t &pmb_nr);\n> \n> \t\t\tif (contiguous && pmb_nr && moved_symbol == l->s)\n> -\t\t\t\tflipped_block = (flipped_block + 1) % 2;\n> +\t\t\t\tflipped_block ^= 1;\n> \t\t\telse\n> \t\t\t\tflipped_block = 0;\n\nThis one I do not see any problem with changing, though I think it is a\nmatter of opinion on which is more readable (I actually tend to think of\n\"x = 0 - x\" as idiomatic for flipping).\n\n> diff --git a/ident.c b/ident.c\n> index cc7afdbf819..188826eed63 100644\n> --- a/ident.c\n> +++ b/ident.c\n> @@ -459,7 +459,7 @@ const char *fmt_ident(const char *name, const char *email,\n> \tint want_name = !(flag & IDENT_NO_NAME);\n> \n> \tstruct strbuf *ident = &ident_pool[index];\n> -\tindex = (index + 1) % ARRAY_SIZE(ident_pool);\n> +\tindex ^= 1;\n> \n> \tif (!email) {\n> \t\tif (whose_ident == WANT_AUTHOR_IDENT && git_author_email.len)\n\nThis has the same problem as the first case.\n\n> diff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c\n> index 70396fa3845..241136148a5 100644\n> --- a/t/helper/test-path-utils.c\n> +++ b/t/helper/test-path-utils.c\n> @@ -185,7 +185,7 @@ static int check_dotfile(const char *x, const char **argv,\n> \tint res = 0, expect = 1;\n> \tfor (; *argv; argv++) {\n> \t\tif (!strcmp(\"--not\", *argv))\n> -\t\t\texpect = !expect;\n> +\t\t\texpect ^= 1;\n\nThis one is not wrong, but IMHO it is more clear to express negation of\na boolean using \"!\" (i.e., what the code is already doing).\n\n\nSo of the four hunks, only the second one seems like a possible\nimprovement, and even there I am not sure the readability is better.\n\n-Peff\n"},{"id":"485598","messageId":"8bea38fe-38a3-412a-b189-541a6596d623@web.de","threadId":"60608","inReplyTo":"20231212200920.GC1127366@coredump.intra.peff.net","subject":"Re: [PATCH] Use ^=1 to toggle between 0 and 1","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-12-12T22:30:03Z","receivedAt":"2023-12-12T22:30:54Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 12.12.23 um 21:09 schrieb Jeff King:\n> On Tue, Dec 12, 2023 at 05:17:47PM +0000, AtariDreams via GitGitGadget wrote:\n>\n>> diff --git a/diff.c b/diff.c\n>> index 2c602df10a3..91842b54753 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -1191,7 +1191,7 @@ static void mark_color_as_moved(struct diff_options *o,\n>> \t\t\t\t\t\t\t &pmb_nr);\n>>\n>> \t\t\tif (contiguous && pmb_nr && moved_symbol == l->s)\n>> -\t\t\t\tflipped_block = (flipped_block + 1) % 2;\n>> +\t\t\t\tflipped_block ^= 1;\n>> \t\t\telse\n>> \t\t\t\tflipped_block = 0;\n>\n> This one I do not see any problem with changing, though I think it is a\n> matter of opinion on which is more readable (I actually tend to think of\n> \"x = 0 - x\" as idiomatic for flipping).\n\nDid you mean \"x = 1 - x\"?\n\n x 0 - x 1 - x\n -- ----- -----\n -1 +1 +2\n 0 0 +1\n +1 -1 0\n\nI don't particular like this; it repeats x and seems error-prone. ;-)\n\nI agree with your assessment of the other three cases in the patch.\n\nCan we salvage something from this bikeshedding exercise? I wonder if\nit's time to use the C99 type _Bool in our code. It would allow\ndocumenting that only two possible values exist in cases like the one\nabove. That would be even more useful for function returns, I assume.\n\nRené\n\n"},{"id":"485608","messageId":"20231213080143.GA1684525@coredump.intra.peff.net","threadId":"60608","inReplyTo":"8bea38fe-38a3-412a-b189-541a6596d623@web.de","subject":"Re: [PATCH] Use ^=1 to toggle between 0 and 1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-13T08:01:43Z","receivedAt":"2023-12-13T08:01:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 12, 2023 at 11:30:03PM +0100, René Scharfe wrote:\n\n> Am 12.12.23 um 21:09 schrieb Jeff King:\n> > On Tue, Dec 12, 2023 at 05:17:47PM +0000, AtariDreams via GitGitGadget wrote:\n> >\n> >> diff --git a/diff.c b/diff.c\n> >> index 2c602df10a3..91842b54753 100644\n> >> --- a/diff.c\n> >> +++ b/diff.c\n> >> @@ -1191,7 +1191,7 @@ static void mark_color_as_moved(struct diff_options *o,\n> >> \t\t\t\t\t\t\t &pmb_nr);\n> >>\n> >> \t\t\tif (contiguous && pmb_nr && moved_symbol == l->s)\n> >> -\t\t\t\tflipped_block = (flipped_block + 1) % 2;\n> >> +\t\t\t\tflipped_block ^= 1;\n> >> \t\t\telse\n> >> \t\t\t\tflipped_block = 0;\n> >\n> > This one I do not see any problem with changing, though I think it is a\n> > matter of opinion on which is more readable (I actually tend to think of\n> > \"x = 0 - x\" as idiomatic for flipping).\n> \n> Did you mean \"x = 1 - x\"?\n\nOops, yes, of course. I'm not sure how I managed to fumble that.\n\n> I don't particular like this; it repeats x and seems error-prone. ;-)\n\nYes. :)\n\nWithout digging into the code, I had just assumed that flipped_block was\nused as an array index. But it really is a boolean, so I actually think\n\"flipped_block = !flipped_block\" would probably be the most clear (but\nIMHO not really worth the churn).\n\n> Can we salvage something from this bikeshedding exercise? I wonder if\n> it's time to use the C99 type _Bool in our code. It would allow\n> documenting that only two possible values exist in cases like the one\n> above. That would be even more useful for function returns, I assume.\n\nHmm, possibly. I guess that might have helped my confusion, and I do\nthink returning bool for function returns would help make their meaning\nmore clear (it would help distinguish them from the usual \"0 for\nsuccess\" return values).\n\nI don't even know that we'd need much of a weather-balloon patch. I\nthink it would be valid to do:\n\n #ifndef bool\n #define bool int\n\nto handle pre-C99 compilers (if there even are any these days). Of\ncourse we probably need some conditional magic to try to \"#include\n\" for the actual C99. I guess we could assume C99 by default\nand then add NO_STDBOOL as an escape hatch if anybody complains.\n\n-Peff\n"},{"id":"485618","messageId":"xmqqil52nndf.fsf@gitster.g","threadId":"60608","inReplyTo":"20231213080143.GA1684525@coredump.intra.peff.net","subject":"Re: [PATCH] Use ^=1 to toggle between 0 and 1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-13T15:17:00Z","receivedAt":"2023-12-13T15:17:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Without digging into the code, I had just assumed that flipped_block was\n> used as an array index. But it really is a boolean, so I actually think\n> \"flipped_block = !flipped_block\" would probably be the most clear (but\n> IMHO not really worth the churn).\n\n;-)\n\n> I don't even know that we'd need much of a weather-balloon patch. I\n> think it would be valid to do:\n>\n> #ifndef bool\n> #define bool int\n>\n> to handle pre-C99 compilers (if there even are any these days). Of\n> course we probably need some conditional magic to try to \"#include\n> \" for the actual C99. I guess we could assume C99 by default\n> and then add NO_STDBOOL as an escape hatch if anybody complains.\n\nSounds good.\n\n"},{"id":"485642","messageId":"4d0b2a5f-305b-4350-b164-44923cb250d8@web.de","threadId":"60608","inReplyTo":"20231213080143.GA1684525@coredump.intra.peff.net","subject":"Re: [PATCH] Use ^=1 to toggle between 0 and 1","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-12-14T13:08:31Z","receivedAt":"2023-12-14T13:08:40Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 13.12.23 um 09:01 schrieb Jeff King:\n> On Tue, Dec 12, 2023 at 11:30:03PM +0100, René Scharfe wrote:\n>\n>> I wonder if\n>> it's time to use the C99 type _Bool in our code. It would allow\n>> documenting that only two possible values exist in cases like the one\n>> above. That would be even more useful for function returns, I assume.\n\n> I don't even know that we'd need much of a weather-balloon patch. I\n> think it would be valid to do:\n>\n> #ifndef bool\n> #define bool int\n>\n> to handle pre-C99 compilers (if there even are any these days). Of\n> course we probably need some conditional magic to try to \"#include\n> \" for the actual C99. I guess we could assume C99 by default\n> and then add NO_STDBOOL as an escape hatch if anybody complains.\n\nThe semantics are slightly different in edge cases, so that fallback\nwould not be fully watertight. E.g. consider:\n\n bool b(bool cond) {return cond == true;}\n bool b2(void) {return b(2);}\n\nb() returns false if you give it false and true for anything else. b2()\nreturns true.\n\nWith int as the fallback this becomes:\n\n int b(int cond) {return cond == 1;}\n int b2(void) {return b(2);}\n\nNow only 1 is recognized as true, b2() returns 0 (false).\n\nA coding rule to not compare bools could mitigate that. Or a rule to\nonly use the values true and false in bool context and to only use\nlogical operators on them.\n\nRené\n"},{"id":"485670","messageId":"20231214220503.GA3320432@coredump.intra.peff.net","threadId":"60608","inReplyTo":"4d0b2a5f-305b-4350-b164-44923cb250d8@web.de","subject":"Re: [PATCH] Use ^=1 to toggle between 0 and 1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-14T22:05:03Z","receivedAt":"2023-12-14T22:05:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 14, 2023 at 02:08:31PM +0100, René Scharfe wrote:\n\n> > I don't even know that we'd need much of a weather-balloon patch. I\n> > think it would be valid to do:\n> >\n> > #ifndef bool\n> > #define bool int\n> >\n> > to handle pre-C99 compilers (if there even are any these days). Of\n> > course we probably need some conditional magic to try to \"#include\n> > \" for the actual C99. I guess we could assume C99 by default\n> > and then add NO_STDBOOL as an escape hatch if anybody complains.\n> \n> The semantics are slightly different in edge cases, so that fallback\n> would not be fully watertight. E.g. consider:\n> \n> bool b(bool cond) {return cond == true;}\n> bool b2(void) {return b(2);}\n\nYeah. b2() is wrong for passing \"2\" to a bool. I assumed that the\ncompiler would warn of that (at least for people on modern C99\ncompilers, not the fallback code), but it doesn't seem to. It's been a\nlong time since I've worked on a code base that made us of \"bool\", but I\nguess that idea is that silently coercing a non-zero int to a bool is\nreasonable in many cases (e.g., \"bool found_foo = count_foos()\").\n\nI guess one could argue that b() is also sub-optimal, as it should just\nsay \"return cond\" or \"return !cond\" rather than explicitly comparing to\ntrue/false. But I won't be surprised if it happens from time to time.\n\n> A coding rule to not compare bools could mitigate that. Or a rule to\n> only use the values true and false in bool context and to only use\n> logical operators on them.\n\nThat seems more complex than we want if our goal is just supporting\nlegacy systems that may or may not even exist. Given your example, I'd\nbe more inclined to just do a weather-balloon adding to\ngit-compat-util.h, and using \"bool\" in a single spot in the code. If\nnobody screams after a few releases, we can consider it OK. If they do,\nit's a trivial patch to convert back.\n\n-Peff\n"},{"id":"485717","messageId":"99b3a727-36fd-4fa5-a6be-60ae6fc5911e@gmail.com","threadId":"60608","inReplyTo":"20231214220503.GA3320432@coredump.intra.peff.net","subject":"Re: [PATCH] Use ^=1 to toggle between 0 and 1","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-12-15T14:46:36Z","receivedAt":"2023-12-15T14:46:39Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 14/12/2023 22:05, Jeff King wrote:\n> On Thu, Dec 14, 2023 at 02:08:31PM +0100, René Scharfe wrote:\n> \n>>> I don't even know that we'd need much of a weather-balloon patch. I\n>>> think it would be valid to do:\n>>>\n>>> #ifndef bool\n>>> #define bool int\n>>>\n>>> to handle pre-C99 compilers (if there even are any these days). Of\n>>> course we probably need some conditional magic to try to \"#include\n>>> \" for the actual C99. I guess we could assume C99 by default\n>>> and then add NO_STDBOOL as an escape hatch if anybody complains.\n>>\n>> The semantics are slightly different in edge cases, so that fallback\n>> would not be fully watertight. E.g. consider:\n>>\n>> bool b(bool cond) {return cond == true;}\n>> bool b2(void) {return b(2);}\n\nThanks for bring this up René, I had similar concerns when I saw the \nsuggestion of using \"int\" as a fallback.\n\n> Yeah. b2() is wrong for passing \"2\" to a bool.\n\nI think it depends what you mean by \"wrong\" §6.3.1.2 of standard is \nquite clear that when any non-zero scalar value is converted to _Bool \nthe result is \"1\"\n\n> I assumed that the\n> compiler would warn of that (at least for people on modern C99\n> compilers, not the fallback code), but it doesn't seem to. It's been a\n> long time since I've worked on a code base that made us of \"bool\", but I\n> guess that idea is that silently coercing a non-zero int to a bool is\n> reasonable in many cases (e.g., \"bool found_foo = count_foos()\").\n\nI guess it is also consistent with the way \"if\" and \"while\" consider a \nnon-zero scalar value to be \"true\".\n\n> I guess one could argue that b() is also sub-optimal, as it should just\n> say \"return cond\" or \"return !cond\" rather than explicitly comparing to\n> true/false. But I won't be surprised if it happens from time to time.\n\nEven if it unlikely that we would directly compare a boolean variable to \n\"true\" or \"false\" it is certainly conceivable that we'd compare two \nboolean variables directly. For the integer fallback to be safe we'd \nneed to write\n\n\tif (!cond_a == !cond_b)\n\nrather than\n\n\tif (cond_a == cond_b)\n\n>> A coding rule to not compare bools could mitigate that. Or a rule to\n>> only use the values true and false in bool context and to only use\n>> logical operators on them.\n> \n> That seems more complex than we want if our goal is just supporting\n> legacy systems that may or may not even exist. Given your example, I'd\n> be more inclined to just do a weather-balloon adding to\n> git-compat-util.h, and using \"bool\" in a single spot in the code. If\n> nobody screams after a few releases, we can consider it OK. If they do,\n> it's a trivial patch to convert back.\n\nA weather-balloon seems like the safest route forward. We have been \nrequiring C99 for two years now [1], hopefully there aren't any \ncompilers out that claim to support C99 but don't provide \"\" \n(I did check online and the compiler on NonStop does support _Bool).\n\nBest Wishes\n\nPhillip\n\n[1] 7bc341e21b (git-compat-util: add a test balloon for C99 support, \n2021-12-01)\n"},{"id":"485722","messageId":"xmqqo7erl7er.fsf@gitster.g","threadId":"60608","inReplyTo":"99b3a727-36fd-4fa5-a6be-60ae6fc5911e@gmail.com","subject":"Re: [PATCH] Use ^=1 to toggle between 0 and 1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-15T17:09:16Z","receivedAt":"2023-12-15T17:09:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Even if it unlikely that we would directly compare a boolean variable\n> to \"true\" or \"false\" it is certainly conceivable that we'd compare two\n> boolean variables directly. For the integer fallback to be safe we'd\n> need to write\n>\n> \tif (!cond_a == !cond_b)\n>\n> rather than\n>\n> \tif (cond_a == cond_b)\n\nEek, it defeats the benefit of using true Boolean type if we had to\ntrain ourselves to write the former, doesn't it?\n\n> A weather-balloon seems like the safest route forward. We have been\n> requiring C99 for two years now [1], hopefully there aren't any\n> compilers out that claim to support C99 but don't provide\n> \"\" (I did check online and the compiler on NonStop does\n> support _Bool).\n>\n> Best Wishes\n>\n> Phillip\n>\n> [1] 7bc341e21b (git-compat-util: add a test balloon for C99 support,\n> 2021-12-01)\n\nNice to be reminded of this one.\n\nThe cited commit does not start to use any specific feature from\nC99, other than that we now require that the compiler claims C99\nconformance by __STDC_VERSION__ set appropriately. The commit log\nmessage says C99 \"provides a variety of useful features, including\n..., many of which we already use.\", which implies that our wish was\nto officially allow any and all features in C99 to be used in our\ncodebase after a successful flight of this test balloon.\n\nNow, I think we saw a successful flight of this test balloon by now.\nIs allowing all the C99 the next step we really want to take?\n\nI still personally have an aversion against decl-after-statement and\n//-comments, not due to portability reasons at all, but because I\nfind that the code is easier to read without it. But in principle,\nit is powerful to be able to say \"OK, as long as the feature is in\nC99 you can use it\", instead of having to decide on individual\nfeatures, and I am not fundamentally against going that route if it\nis where people want to go.\n\nThanks.\n\n"},{"id":"485743","messageId":"b3c0c52b-7e12-4068-99fd-07fe4fa3da7d@web.de","threadId":"60608","inReplyTo":"xmqqo7erl7er.fsf@gitster.g","subject":"Re: [PATCH] Use ^=1 to toggle between 0 and 1","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-12-16T10:46:30Z","receivedAt":"2023-12-16T10:46:44Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 15.12.23 um 18:09 schrieb Junio C Hamano:\n> Phillip Wood writes:\n>\n>> Even if it unlikely that we would directly compare a boolean variable\n>> to \"true\" or \"false\" it is certainly conceivable that we'd compare two\n>> boolean variables directly. For the integer fallback to be safe we'd\n>> need to write\n>>\n>> \tif (!cond_a == !cond_b)\n>>\n>> rather than\n>>\n>> \tif (cond_a == cond_b)\n>\n> Eek, it defeats the benefit of using true Boolean type if we had to\n> train ourselves to write the former, doesn't it?\n\nIndeed.\n\n>> [1] 7bc341e21b (git-compat-util: add a test balloon for C99 support,\n>> 2021-12-01)\n>\n> Nice to be reminded of this one.\n>\n> The cited commit does not start to use any specific feature from\n> C99, other than that we now require that the compiler claims C99\n> conformance by __STDC_VERSION__ set appropriately. The commit log\n> message says C99 \"provides a variety of useful features, including\n> ..., many of which we already use.\", which implies that our wish was\n> to officially allow any and all features in C99 to be used in our\n> codebase after a successful flight of this test balloon.\n>\n> Now, I think we saw a successful flight of this test balloon by now.\n> Is allowing all the C99 the next step we really want to take?\n>\n> I still personally have an aversion against decl-after-statement and\n> //-comments, not due to portability reasons at all, but because I\n> find that the code is easier to read without it. But in principle,\n> it is powerful to be able to say \"OK, as long as the feature is in\n> C99 you can use it\", instead of having to decide on individual\n> features, and I am not fundamentally against going that route if it\n> is where people want to go.\n\nC99 added a lot of features, but we already use several of them.\nSupport for individual features may vary, though -- who knows?\n\nE.g. http://www.compilers.de/vbcc.html claims to support \"most of\nISO/IEC 9899:1999 (C99)\", yet _Bool is not mentioned in its docs (but\n__STDC_VERSION__ 199901L is). It's not a particularly interesting\ncompiler for us, but still a real-world example of selective C99\nsupport.\n\nThe table at the bottom of https://en.cppreference.com/w/c/99 would be\nuseful if it was filled out for more compilers. And it also doesn't\nmention _Bool and stdbool.h.\n\nTenDRA and the M/o/Vfuscator are the only compilers without stdbool.h\nsupport on https://godbolt.org/ as far as I can see, but that website\ndoesn't have a lot of commercial compilers (understandably).\n\nSo I guess in practice we still need to check each new feature, even\nthough in theory we should be fine after the two-year test.\n\nRené\n"},{"id":"485744","messageId":"2d30dc36-6091-4b47-846f-92d3f4a8b135@web.de","threadId":"60608","inReplyTo":"99b3a727-36fd-4fa5-a6be-60ae6fc5911e@gmail.com","subject":"[PATCH] git-compat-util: convert skip_{prefix,suffix}{,_mem} to bool","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-12-16T10:47:21Z","receivedAt":"2023-12-16T10:47:33Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Use the data type bool and its values true and false to document the\nbinary return value of skip_prefix() and friends more explicitly.\n\nThis first use of stdbool.h, introduced with C99, is meant to check\nwhether there are platforms that claim support for C99, as tested by\n7bc341e21b (git-compat-util: add a test balloon for C99 support,\n2021-12-01), but still lack that header for some reason.\n\nA fallback based on a wider type, e.g. int, would have to deal with\ncomparisons somehow to emulate that any non-zero value is true:\n\n bool b1 = 1;\n bool b2 = 2;\n if (b1 == b2) puts(\"This is true.\");\n\n int i1 = 1;\n int i2 = 2;\n if (i1 == i2) puts(\"Not printed.\");\n #define BOOLEQ(a, b) (!(a) == !(b))\n if (BOOLEQ(i1, i2)) puts(\"This is true.\");\n\nSo we'd be better off using bool everywhere without a fallback, if\npossible. That's why this patch doesn't include any.\n\nSigned-off-by: René Scharfe \n---\n git-compat-util.h | 42 ++++++++++++++++++++++--------------------\n 1 file changed, 22 insertions(+), 20 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 3e7a59b5ff..603c97e3b3 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -225,6 +225,7 @@ struct strbuf;\n #include \n #include \n #include \n+#include \n #include \n #ifdef HAVE_STRINGS_H\n #include /* for strcasecmp() */\n@@ -684,11 +685,11 @@ report_fn get_warn_routine(void);\n void set_die_is_recursing_routine(int (*routine)(void));\n\n /*\n- * If the string \"str\" begins with the string found in \"prefix\", return 1.\n+ * If the string \"str\" begins with the string found in \"prefix\", return true.\n * The \"out\" parameter is set to \"str + strlen(prefix)\" (i.e., to the point in\n * the string right after the prefix).\n *\n- * Otherwise, return 0 and leave \"out\" untouched.\n+ * Otherwise, return false and leave \"out\" untouched.\n *\n * Examples:\n *\n@@ -699,57 +700,58 @@ void set_die_is_recursing_routine(int (*routine)(void));\n * [skip prefix if present, otherwise use whole string]\n * skip_prefix(name, \"refs/heads/\", &name);\n */\n-static inline int skip_prefix(const char *str, const char *prefix,\n-\t\t\t const char **out)\n+static inline bool skip_prefix(const char *str, const char *prefix,\n+\t\t\t const char **out)\n {\n \tdo {\n \t\tif (!*prefix) {\n \t\t\t*out = str;\n-\t\t\treturn 1;\n+\t\t\treturn true;\n \t\t}\n \t} while (*str++ == *prefix++);\n-\treturn 0;\n+\treturn false;\n }\n\n /*\n * Like skip_prefix, but promises never to read past \"len\" bytes of the input\n * buffer, and returns the remaining number of bytes in \"out\" via \"outlen\".\n */\n-static inline int skip_prefix_mem(const char *buf, size_t len,\n-\t\t\t\t const char *prefix,\n-\t\t\t\t const char **out, size_t *outlen)\n+static inline bool skip_prefix_mem(const char *buf, size_t len,\n+\t\t\t\t const char *prefix,\n+\t\t\t\t const char **out, size_t *outlen)\n {\n \tsize_t prefix_len = strlen(prefix);\n \tif (prefix_len <= len && !memcmp(buf, prefix, prefix_len)) {\n \t\t*out = buf + prefix_len;\n \t\t*outlen = len - prefix_len;\n-\t\treturn 1;\n+\t\treturn true;\n \t}\n-\treturn 0;\n+\treturn false;\n }\n\n /*\n- * If buf ends with suffix, return 1 and subtract the length of the suffix\n- * from *len. Otherwise, return 0 and leave *len untouched.\n+ * If buf ends with suffix, return true and subtract the length of the suffix\n+ * from *len. Otherwise, return false and leave *len untouched.\n */\n-static inline int strip_suffix_mem(const char *buf, size_t *len,\n-\t\t\t\t const char *suffix)\n+static inline bool strip_suffix_mem(const char *buf, size_t *len,\n+\t\t\t\t const char *suffix)\n {\n \tsize_t suflen = strlen(suffix);\n \tif (*len < suflen || memcmp(buf + (*len - suflen), suffix, suflen))\n-\t\treturn 0;\n+\t\treturn false;\n \t*len -= suflen;\n-\treturn 1;\n+\treturn true;\n }\n\n /*\n- * If str ends with suffix, return 1 and set *len to the size of the string\n- * without the suffix. Otherwise, return 0 and set *len to the size of the\n+ * If str ends with suffix, return true and set *len to the size of the string\n+ * without the suffix. Otherwise, return false and set *len to the size of the\n * string.\n *\n * Note that we do _not_ NUL-terminate str to the new length.\n */\n-static inline int strip_suffix(const char *str, const char *suffix, size_t *len)\n+static inline bool strip_suffix(const char *str, const char *suffix,\n+\t\t\t\tsize_t *len)\n {\n \t*len = strlen(str);\n \treturn strip_suffix_mem(str, len, suffix);\n--\n2.43.0\n"},{"id":"485780","messageId":"ff4a6abc-8abf-4dbf-a787-e4895a78b048@gmail.com","threadId":"60608","inReplyTo":"xmqqo7erl7er.fsf@gitster.g","subject":"Re: [PATCH] Use ^=1 to toggle between 0 and 1","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-12-18T16:18:36Z","receivedAt":"2023-12-18T16:18:40Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 15/12/2023 17:09, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>> Even if it unlikely that we would directly compare a boolean variable\n>> to \"true\" or \"false\" it is certainly conceivable that we'd compare two\n>> boolean variables directly. For the integer fallback to be safe we'd\n>> need to write\n>>\n>> \tif (!cond_a == !cond_b)\n>>\n>> rather than\n>>\n>> \tif (cond_a == cond_b)\n> \n> Eek, it defeats the benefit of using true Boolean type if we had to\n> train ourselves to write the former, doesn't it?\n\nYes, it's horrible - if for some reason it turns out that we cannot use \n\"#include \" everywhere I think we should drop it rather than \nproviding a subtly incompatible fallback\n\n>> A weather-balloon seems like the safest route forward. We have been\n>> requiring C99 for two years now [1], hopefully there aren't any\n>> compilers out that claim to support C99 but don't provide\n>> \"\" (I did check online and the compiler on NonStop does\n>> support _Bool).\n>>\n>> Best Wishes\n>>\n>> Phillip\n>>\n>> [1] 7bc341e21b (git-compat-util: add a test balloon for C99 support,\n>> 2021-12-01)\n> \n> Nice to be reminded of this one.\n> \n> The cited commit does not start to use any specific feature from\n> C99, other than that we now require that the compiler claims C99\n> conformance by __STDC_VERSION__ set appropriately. The commit log\n> message says C99 \"provides a variety of useful features, including\n> ..., many of which we already use.\", which implies that our wish was\n> to officially allow any and all features in C99 to be used in our\n> codebase after a successful flight of this test balloon.\n> \n> Now, I think we saw a successful flight of this test balloon by now.\n> Is allowing all the C99 the next step we really want to take?\n >\n> I still personally have an aversion against decl-after-statement and\n> //-comments, not due to portability reasons at all, but because I\n> find that the code is easier to read without it. But in principle,\n> it is powerful to be able to say \"OK, as long as the feature is in\n> C99 you can use it\", instead of having to decide on individual\n> features, and I am not fundamentally against going that route if it\n> is where people want to go.\n\nI'm not sure we necessarily want to say \"use anything that is in C99\" \nfor several reasons.\n\n - Some features such as C99's variable length arrays are known to be\n problematic.\n\n - As you say above there maybe features that we think harm the\n readability of our code.\n\n - As René points out not all compilers necessarily support all\n features.\n\nI think using _Bool could be useful for the reasons Peff outlined. As \nfor other features I've written code that I think would have benefited \nfrom compound literals, but off the top of my head I can't think of any \nother C99 features that I personally wish we were using. I think that \ndecl-after-statement is occasionally useful to declare a variable near \nwhere it is used in a long function body but it is much simpler just to \nban it altogether and encourage people to break up long functions to \nmake them more readable.\n\nBest Wishes\n\nPhillip\n\n> Thanks.\n> \n> \n"},{"id":"485781","messageId":"b7a56625-46d5-4d77-b4bf-5595a6fb2aef@gmail.com","threadId":"60608","inReplyTo":"2d30dc36-6091-4b47-846f-92d3f4a8b135@web.de","subject":"Re: [PATCH] git-compat-util: convert skip_{prefix,suffix}{,_mem} to bool","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-12-18T16:23:19Z","receivedAt":"2023-12-18T16:23:23Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi René\n\nOn 16/12/2023 10:47, René Scharfe wrote:\n> Use the data type bool and its values true and false to document the\n> binary return value of skip_prefix() and friends more explicitly.\n> \n> This first use of stdbool.h, introduced with C99, is meant to check\n> whether there are platforms that claim support for C99, as tested by\n> 7bc341e21b (git-compat-util: add a test balloon for C99 support,\n> 2021-12-01), but still lack that header for some reason.\n> \n> A fallback based on a wider type, e.g. int, would have to deal with\n> comparisons somehow to emulate that any non-zero value is true:\n> \n> bool b1 = 1;\n> bool b2 = 2;\n> if (b1 == b2) puts(\"This is true.\");\n> \n> int i1 = 1;\n> int i2 = 2;\n> if (i1 == i2) puts(\"Not printed.\");\n> #define BOOLEQ(a, b) (!(a) == !(b))\n> if (BOOLEQ(i1, i2)) puts(\"This is true.\");\n> \n> So we'd be better off using bool everywhere without a fallback, if\n> possible. That's why this patch doesn't include any.\n\nThanks for the comprehensive commit message, I agree that we'd be better \noff avoiding adding a fallback. The patch looks good, I did wonder if we \nreally need to covert all of these functions for a test-balloon but the \npatch is still pretty small overall.\n\nBest Wishes\n\nPhillip\n\n> Signed-off-by: René Scharfe \n> ---\n> git-compat-util.h | 42 ++++++++++++++++++++++--------------------\n> 1 file changed, 22 insertions(+), 20 deletions(-)\n> \n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index 3e7a59b5ff..603c97e3b3 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -225,6 +225,7 @@ struct strbuf;\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #ifdef HAVE_STRINGS_H\n> #include /* for strcasecmp() */\n> @@ -684,11 +685,11 @@ report_fn get_warn_routine(void);\n> void set_die_is_recursing_routine(int (*routine)(void));\n> \n> /*\n> - * If the string \"str\" begins with the string found in \"prefix\", return 1.\n> + * If the string \"str\" begins with the string found in \"prefix\", return true.\n> * The \"out\" parameter is set to \"str + strlen(prefix)\" (i.e., to the point in\n> * the string right after the prefix).\n> *\n> - * Otherwise, return 0 and leave \"out\" untouched.\n> + * Otherwise, return false and leave \"out\" untouched.\n> *\n> * Examples:\n> *\n> @@ -699,57 +700,58 @@ void set_die_is_recursing_routine(int (*routine)(void));\n> * [skip prefix if present, otherwise use whole string]\n> * skip_prefix(name, \"refs/heads/\", &name);\n> */\n> -static inline int skip_prefix(const char *str, const char *prefix,\n> -\t\t\t const char **out)\n> +static inline bool skip_prefix(const char *str, const char *prefix,\n> +\t\t\t const char **out)\n> {\n> \tdo {\n> \t\tif (!*prefix) {\n> \t\t\t*out = str;\n> -\t\t\treturn 1;\n> +\t\t\treturn true;\n> \t\t}\n> \t} while (*str++ == *prefix++);\n> -\treturn 0;\n> +\treturn false;\n> }\n> \n> /*\n> * Like skip_prefix, but promises never to read past \"len\" bytes of the input\n> * buffer, and returns the remaining number of bytes in \"out\" via \"outlen\".\n> */\n> -static inline int skip_prefix_mem(const char *buf, size_t len,\n> -\t\t\t\t const char *prefix,\n> -\t\t\t\t const char **out, size_t *outlen)\n> +static inline bool skip_prefix_mem(const char *buf, size_t len,\n> +\t\t\t\t const char *prefix,\n> +\t\t\t\t const char **out, size_t *outlen)\n> {\n> \tsize_t prefix_len = strlen(prefix);\n> \tif (prefix_len <= len && !memcmp(buf, prefix, prefix_len)) {\n> \t\t*out = buf + prefix_len;\n> \t\t*outlen = len - prefix_len;\n> -\t\treturn 1;\n> +\t\treturn true;\n> \t}\n> -\treturn 0;\n> +\treturn false;\n> }\n> \n> /*\n> - * If buf ends with suffix, return 1 and subtract the length of the suffix\n> - * from *len. Otherwise, return 0 and leave *len untouched.\n> + * If buf ends with suffix, return true and subtract the length of the suffix\n> + * from *len. Otherwise, return false and leave *len untouched.\n> */\n> -static inline int strip_suffix_mem(const char *buf, size_t *len,\n> -\t\t\t\t const char *suffix)\n> +static inline bool strip_suffix_mem(const char *buf, size_t *len,\n> +\t\t\t\t const char *suffix)\n> {\n> \tsize_t suflen = strlen(suffix);\n> \tif (*len < suflen || memcmp(buf + (*len - suflen), suffix, suflen))\n> -\t\treturn 0;\n> +\t\treturn false;\n> \t*len -= suflen;\n> -\treturn 1;\n> +\treturn true;\n> }\n> \n> /*\n> - * If str ends with suffix, return 1 and set *len to the size of the string\n> - * without the suffix. Otherwise, return 0 and set *len to the size of the\n> + * If str ends with suffix, return true and set *len to the size of the string\n> + * without the suffix. Otherwise, return false and set *len to the size of the\n> * string.\n> *\n> * Note that we do _not_ NUL-terminate str to the new length.\n> */\n> -static inline int strip_suffix(const char *str, const char *suffix, size_t *len)\n> +static inline bool strip_suffix(const char *str, const char *suffix,\n> +\t\t\t\tsize_t *len)\n> {\n> \t*len = strlen(str);\n> \treturn strip_suffix_mem(str, len, suffix);\n> --\n> 2.43.0\n> \n"},{"id":"485793","messageId":"xmqqa5q7e00q.fsf@gitster.g","threadId":"60608","inReplyTo":"b7a56625-46d5-4d77-b4bf-5595a6fb2aef@gmail.com","subject":"Re: [PATCH] git-compat-util: convert skip_{prefix,suffix}{,_mem} to bool","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-18T20:19:49Z","receivedAt":"2023-12-18T20:20:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Thanks for the comprehensive commit message, I agree that we'd be\n> better off avoiding adding a fallback. The patch looks good, I did\n> wonder if we really need to covert all of these functions for a\n> test-balloon but the patch is still pretty small overall.\n\nI do have to wonder, though, if we want to be a bit more careful\nthan just blindly trusting the platform (i.e. might\nexist and __STDC_VERSION__ may say C99, but under the hood their\nimplementation may be buggy and coerce the result of an assignment\nof 2 to be different from assigning true).\n\nIn any case, this is a good starting place. Let's queue it, see\nwhat happens, and then think about longer-term plans.\n\nThanks.\n"},{"id":"485812","messageId":"aa9e4e07-03d8-4f4e-a63c-f393e1b56c92@web.de","threadId":"60608","inReplyTo":"xmqqa5q7e00q.fsf@gitster.g","subject":"Re: [PATCH] git-compat-util: convert skip_{prefix,suffix}{,_mem} to bool","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-12-19T13:36:01Z","receivedAt":"2023-12-19T13:36:26Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 18.12.23 um 21:19 schrieb Junio C Hamano:\n> Phillip Wood writes:\n>\n>> Thanks for the comprehensive commit message, I agree that we'd be\n>> better off avoiding adding a fallback. The patch looks good, I did\n>> wonder if we really need to covert all of these functions for a\n>> test-balloon but the patch is still pretty small overall.\n>\n> I do have to wonder, though, if we want to be a bit more careful\n> than just blindly trusting the platform (i.e. might\n> exist and __STDC_VERSION__ may say C99, but under the hood their\n> implementation may be buggy and coerce the result of an assignment\n> of 2 to be different from assigning true).\n\nWe could add a compile-time check like below. I can't decide if this\nwould be prudent or paranoid. It's cheap, though, so perhaps just add\nthis tripwire for non-conforming compilers without making a judgement?\n\nRené\n\n\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 603c97e3b3..8212feaa37 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -705,7 +705,7 @@ static inline bool skip_prefix(const char *str, const char *prefix,\n {\n \tdo {\n \t\tif (!*prefix) {\n-\t\t\t*out = str;\n+\t\t\t*out = str + BUILD_ASSERT_OR_ZERO((bool)1 == (bool)2);\n \t\t\treturn true;\n \t\t}\n \t} while (*str++ == *prefix++);\n"},{"id":"485913","messageId":"20231221095606.GB570888@coredump.intra.peff.net","threadId":"60608","inReplyTo":"99b3a727-36fd-4fa5-a6be-60ae6fc5911e@gmail.com","subject":"Re: [PATCH] Use ^=1 to toggle between 0 and 1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-21T09:56:06Z","receivedAt":"2023-12-21T09:56:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 15, 2023 at 02:46:36PM +0000, Phillip Wood wrote:\n\n> > Yeah. b2() is wrong for passing \"2\" to a bool.\n> \n> I think it depends what you mean by \"wrong\" §6.3.1.2 of standard is quite\n> clear that when any non-zero scalar value is converted to _Bool the result\n> is \"1\"\n\nYeah, sorry, I was being quite sloppy with my wording. I meant \"wrong\"\nas in \"I would ideally flag this in review for being weird and\nconfusing\".\n\nOf course there are many reasonable cases where you might pass an\ninteger \"foo\" rather than explicitly booleanizing it with \"!!foo\". So I\ndo agree it's a real potential problem (and I'm sufficiently convinced\nthat we should avoid an \"int\" fallback if we can).\n\n-Peff\n"},{"id":"485914","messageId":"20231221095907.GC570888@coredump.intra.peff.net","threadId":"60608","inReplyTo":"2d30dc36-6091-4b47-846f-92d3f4a8b135@web.de","subject":"Re: [PATCH] git-compat-util: convert skip_{prefix,suffix}{,_mem} to bool","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-21T09:59:07Z","receivedAt":"2023-12-21T09:59:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 16, 2023 at 11:47:21AM +0100, René Scharfe wrote:\n\n> Use the data type bool and its values true and false to document the\n> binary return value of skip_prefix() and friends more explicitly.\n> \n> This first use of stdbool.h, introduced with C99, is meant to check\n> whether there are platforms that claim support for C99, as tested by\n> 7bc341e21b (git-compat-util: add a test balloon for C99 support,\n> 2021-12-01), but still lack that header for some reason.\n> \n> A fallback based on a wider type, e.g. int, would have to deal with\n> comparisons somehow to emulate that any non-zero value is true:\n> \n> bool b1 = 1;\n> bool b2 = 2;\n> if (b1 == b2) puts(\"This is true.\");\n> \n> int i1 = 1;\n> int i2 = 2;\n> if (i1 == i2) puts(\"Not printed.\");\n> #define BOOLEQ(a, b) (!(a) == !(b))\n> if (BOOLEQ(i1, i2)) puts(\"This is true.\");\n> \n> So we'd be better off using bool everywhere without a fallback, if\n> possible. That's why this patch doesn't include any.\n\nThanks for putting this together. I agree this is the right spot to end\nup for now (and that if for whatever reason we find that some platforms\ncan't handle it, we probably should revert and not try the naive\nfallback).\n\n-Peff\n"},{"id":"485930","messageId":"23d47364-9ec2-4273-8ea5-f106550e125a@gmail.com","threadId":"60608","inReplyTo":"20231221095606.GB570888@coredump.intra.peff.net","subject":"Re: [PATCH] Use ^=1 to toggle between 0 and 1","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-12-21T15:06:01Z","receivedAt":"2023-12-21T15:06:04Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Peff\n\nOn 21/12/2023 09:56, Jeff King wrote:\n> On Fri, Dec 15, 2023 at 02:46:36PM +0000, Phillip Wood wrote:\n> \n>>> Yeah. b2() is wrong for passing \"2\" to a bool.\n>>\n>> I think it depends what you mean by \"wrong\" §6.3.1.2 of standard is quite\n>> clear that when any non-zero scalar value is converted to _Bool the result\n>> is \"1\"\n> \n> Yeah, sorry, I was being quite sloppy with my wording. I meant \"wrong\"\n> as in \"I would ideally flag this in review for being weird and\n> confusing\".\n\nThat makes sense, it certainly is confusing\n\n> Of course there are many reasonable cases where you might pass an\n> integer \"foo\" rather than explicitly booleanizing it with \"!!foo\". So I\n> do agree it's a real potential problem (and I'm sufficiently convinced\n> that we should avoid an \"int\" fallback if we can).\n\nI had a look at gnulib the other day and the list of limitations in the \ndocumentation of their fallback makes it look quite \nunattractive. They helpfully list some compilers where _Bool is not \nimplemented (IRIX, Tru64) or does not work correctly (HP-UX, AIX). As \nfar as I can see all the bug reports cited are from 2003-2006 on \nobsolete compiler versions, hopefully _Bool is better supported these days.\n\nBest Wishes\n\nPhillip\n\n> -Peff\n"},{"id":"509246","messageId":"pull.1620.v2.git.git.1734481009264.gitgitgadget@gmail.com","threadId":"60608","inReplyTo":"pull.1620.git.git.1702401468082.gitgitgadget@gmail.com","subject":"[PATCH v2] Use ^=1 to toggle between 0 and 1","fromName":"AreaZR via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-12-18T00:16:49Z","receivedAt":"2024-12-18T00:16:53Z","isPatch":true,"sender":{"key":"name:AreaZR","avatar":null},"body":"From: Seija Kijin \n\nIf it is known that an int is either 1 or 0,\ndoing an exclusive or to switch instead of a\nmodulus makes more sense and is more efficient.\n\nSigned-off-by: Seija \n---\n Use ^=1 to toggle between 0 and 1\n \n If it is known that an int is either 1 or 0, doing an exclusive or to\n switch instead of a modulus makes more sense and is more efficient.\n \n Signed-off-by: Seija Kijin doremylover123@gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1620%2FAreaZR%2Fbuffer-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1620/AreaZR/buffer-v2\nPull-Request: https://github.com/git/git/pull/1620\n\nRange-diff vs v1:\n\n 1: 1bdc62e1b19 ! 1: 5819a51526b Use ^=1 to toggle between 0 and 1\n @@ Commit message\n doing an exclusive or to switch instead of a\n modulus makes more sense and is more efficient.\n \n - Signed-off-by: Seija Kijin doremylover123@gmail.com\n -\n - ## builtin/fast-export.c ##\n -@@ builtin/fast-export.c: static void anonymize_ident_line(const char **beg, const char **end)\n - \tstruct ident_split split;\n - \tconst char *end_of_header;\n - \n --\tout = &buffers[which_buffer++];\n --\twhich_buffer %= ARRAY_SIZE(buffers);\n -+\tout = &buffers[which_buffer];\n -+\twhich_buffer ^= 1;\n - \tstrbuf_reset(out);\n - \n - \t/* skip \"committer\", \"author\", \"tagger\", etc */\n + Signed-off-by: Seija \n \n ## diff.c ##\n @@ diff.c: static void mark_color_as_moved(struct diff_options *o,\n @@ diff.c: static void mark_color_as_moved(struct diff_options *o,\n \t\t\t\tflipped_block = 0;\n \n \n - ## ident.c ##\n -@@ ident.c: const char *fmt_ident(const char *name, const char *email,\n - \tint want_name = !(flag & IDENT_NO_NAME);\n - \n - \tstruct strbuf *ident = &ident_pool[index];\n --\tindex = (index + 1) % ARRAY_SIZE(ident_pool);\n -+\tindex ^= 1;\n - \n - \tif (!email) {\n - \t\tif (whose_ident == WANT_AUTHOR_IDENT && git_author_email.len)\n -\n ## t/helper/test-path-utils.c ##\n @@ t/helper/test-path-utils.c: static int check_dotfile(const char *x, const char **argv,\n \tint res = 0, expect = 1;\n\n\n diff.c | 2 +-\n t/helper/test-path-utils.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 266ddf18e73..5c2ac8d6fd1 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1231,7 +1231,7 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\t\t\t\t\t\t &pmb_nr);\n \n \t\t\tif (contiguous && pmb_nr && moved_symbol == l->s)\n-\t\t\t\tflipped_block = (flipped_block + 1) % 2;\n+\t\t\t\tflipped_block ^= 1;\n \t\t\telse\n \t\t\t\tflipped_block = 0;\n \ndiff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c\nindex 3129aa28fd2..0810647c722 100644\n--- a/t/helper/test-path-utils.c\n+++ b/t/helper/test-path-utils.c\n@@ -188,7 +188,7 @@ static int check_dotfile(const char *x, const char **argv,\n \tint res = 0, expect = 1;\n \tfor (; *argv; argv++) {\n \t\tif (!strcmp(\"--not\", *argv))\n-\t\t\texpect = !expect;\n+\t\t\texpect ^= 1;\n \t\telse if (expect != (is_hfs(*argv) || is_ntfs(*argv)))\n \t\t\tres = error(\"'%s' is %s.git%s\", *argv,\n \t\t\t\t expect ? \"not \" : \"\", x);\n\nbase-commit: 2ccc89b0c16c51561da90d21cfbb4b58cc877bf6\n-- \ngitgitgadget\n"},{"id":"509253","messageId":"pull.1620.v3.git.git.1734482536998.gitgitgadget@gmail.com","threadId":"60608","inReplyTo":"pull.1620.v2.git.git.1734481009264.gitgitgadget@gmail.com","subject":"[PATCH v3] git: use ^=1 to toggle between 0 and 1","fromName":"AreaZR via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-12-18T00:42:16Z","receivedAt":"2024-12-18T00:42:20Z","isPatch":true,"sender":{"key":"name:AreaZR","avatar":null},"body":"From: Seija Kijin \n\nIf it is known that an int is either 1 or 0,\ndoing an exclusive or to switch instead of a\nmodulus makes more sense and is more efficient.\n\nSigned-off-by: Seija \n---\n git: use ^=1 to toggle between 0 and 1\n \n If it is known that an int is either 1 or 0, doing an exclusive or to\n switch instead of a modulus makes more sense and is more efficient.\n \n Signed-off-by: Seija Kijin doremylover123@gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1620%2FAreaZR%2Fbuffer-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1620/AreaZR/buffer-v3\nPull-Request: https://github.com/git/git/pull/1620\n\nRange-diff vs v2:\n\n 1: 5819a51526b ! 1: f6e75d8eff0 Use ^=1 to toggle between 0 and 1\n @@ Metadata\n Author: Seija Kijin \n \n ## Commit message ##\n - Use ^=1 to toggle between 0 and 1\n + git: use ^=1 to toggle between 0 and 1\n \n If it is known that an int is either 1 or 0,\n doing an exclusive or to switch instead of a\n\n\n diff.c | 2 +-\n t/helper/test-path-utils.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 266ddf18e73..5c2ac8d6fd1 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1231,7 +1231,7 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\t\t\t\t\t\t &pmb_nr);\n \n \t\t\tif (contiguous && pmb_nr && moved_symbol == l->s)\n-\t\t\t\tflipped_block = (flipped_block + 1) % 2;\n+\t\t\t\tflipped_block ^= 1;\n \t\t\telse\n \t\t\t\tflipped_block = 0;\n \ndiff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c\nindex 3129aa28fd2..0810647c722 100644\n--- a/t/helper/test-path-utils.c\n+++ b/t/helper/test-path-utils.c\n@@ -188,7 +188,7 @@ static int check_dotfile(const char *x, const char **argv,\n \tint res = 0, expect = 1;\n \tfor (; *argv; argv++) {\n \t\tif (!strcmp(\"--not\", *argv))\n-\t\t\texpect = !expect;\n+\t\t\texpect ^= 1;\n \t\telse if (expect != (is_hfs(*argv) || is_ntfs(*argv)))\n \t\t\tres = error(\"'%s' is %s.git%s\", *argv,\n \t\t\t\t expect ? \"not \" : \"\", x);\n\nbase-commit: 063bcebf0c917140ca0e705cbe0fdea127e90086\n-- \ngitgitgadget\n"},{"id":"509266","messageId":"pull.1620.v4.git.git.1734489540328.gitgitgadget@gmail.com","threadId":"60608","inReplyTo":"pull.1620.v3.git.git.1734482536998.gitgitgadget@gmail.com","subject":"[PATCH v4] git: use ^=1 to toggle between 0 and 1","fromName":"AreaZR via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-12-18T02:38:59Z","receivedAt":"2024-12-18T02:39:04Z","isPatch":true,"sender":{"key":"name:AreaZR","avatar":null},"body":"From: Seija Kijin \n\nIf it is known that an int is either 1 or 0,\ndoing an exclusive or to switch instead of a\nmodulus makes more sense and is more efficient.\n\nSigned-off-by: Seija Kijin \n---\n git: use ^=1 to toggle between 0 and 1\n \n If it is known that an int is either 1 or 0, doing an exclusive or to\n switch instead of a modulus makes more sense and is more efficient.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1620%2FAreaZR%2Fbuffer-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1620/AreaZR/buffer-v4\nPull-Request: https://github.com/git/git/pull/1620\n\nRange-diff vs v3:\n\n 1: f6e75d8eff0 ! 1: db1f0b1a323 git: use ^=1 to toggle between 0 and 1\n @@ Commit message\n doing an exclusive or to switch instead of a\n modulus makes more sense and is more efficient.\n \n - Signed-off-by: Seija \n + Signed-off-by: Seija Kijin \n \n ## diff.c ##\n @@ diff.c: static void mark_color_as_moved(struct diff_options *o,\n\n\n diff.c | 2 +-\n t/helper/test-path-utils.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 266ddf18e73..5c2ac8d6fd1 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1231,7 +1231,7 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\t\t\t\t\t\t &pmb_nr);\n \n \t\t\tif (contiguous && pmb_nr && moved_symbol == l->s)\n-\t\t\t\tflipped_block = (flipped_block + 1) % 2;\n+\t\t\t\tflipped_block ^= 1;\n \t\t\telse\n \t\t\t\tflipped_block = 0;\n \ndiff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c\nindex 3129aa28fd2..0810647c722 100644\n--- a/t/helper/test-path-utils.c\n+++ b/t/helper/test-path-utils.c\n@@ -188,7 +188,7 @@ static int check_dotfile(const char *x, const char **argv,\n \tint res = 0, expect = 1;\n \tfor (; *argv; argv++) {\n \t\tif (!strcmp(\"--not\", *argv))\n-\t\t\texpect = !expect;\n+\t\t\texpect ^= 1;\n \t\telse if (expect != (is_hfs(*argv) || is_ntfs(*argv)))\n \t\t\tres = error(\"'%s' is %s.git%s\", *argv,\n \t\t\t\t expect ? \"not \" : \"\", x);\n\nbase-commit: d882f382b3d939d90cfa58d17b17802338f05d66\n-- \ngitgitgadget\n"},{"id":"509305","messageId":"xmqq4j31t2o0.fsf@gitster.g","threadId":"60608","inReplyTo":"pull.1620.v3.git.git.1734482536998.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] git: use ^=1 to toggle between 0 and 1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-18T15:46:07Z","receivedAt":"2024-12-18T15:46:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"AreaZR via GitGitGadget\" writes:\n\n> From: Seija Kijin \n>\n> If it is known that an int is either 1 or 0,\n> doing an exclusive or to switch instead of a\n> modulus makes more sense and is more efficient.\n\nFWIW, it is much more idiomatic in this codebase to say\n\n\tfoo = !foo;\n\nto flip the polarity of foo when foo is used as a Boolean. It is\nboth more readable than toggling only the bottom bit, and it is much\nmore robust. Here 'used as a Boolean' means 'is it zero, or is it\nnon-zero?', the norm used in the C language.\n\nIf the reference to \"foo\", other than the place where the value of\nfoo is consulted for the sole purpose of fliping between true-false,\nwere to check if it is true or not, i.e.\n\n\tif (foo)\n\t\tdo something;\n\telse\n\t\tdo something else;\n\nthen at this \"real\" use site, only the zero-ness of the value\nmatters. foo==0 does something different from foo==1, but the code\nbehaves the same way as the case where foo==1, if foo==2 or foo==3.\n\nBut the code that flips by\n\n\tfoo = 1 - foo;\n\tfoo ^= 1;\n\nmakes an assumption different from and stricter than the real use\nsite. It only allows foo==0 and foo==1 without a good reason.\n\nBut\n\n\tfoo = !foo;\n\nkeeps the same assumption as the real use site, which is why we\nprefer that form.\n\nAnd like it or not, it is natural to assume that 0 is false and\neverything else is true when writing in C, and especially in the\ncodebase of this project. So let's not flip\n\n\tfoo = !foo;\n\ninto\n\n\tfoo ^= 1;\n\njust to make it look different. Going the other way, or rewriting\nrewriting modulo 2 arithmetic into !foo form, would be more\npreferrable.\n\nThe \"flipped_block\" is used like so:\n\n\tif (flipped_block && o->color_moved != COLOR_MOVED_BLOCKS)\n\t\tset MOVED_LINE_ALT bit in flags word;\n\nso it is very much Boolean whose zero-ness matters.\n\n"},{"id":"509307","messageId":"pull.1620.v5.git.git.1734540395021.gitgitgadget@gmail.com","threadId":"60608","inReplyTo":"pull.1620.v4.git.git.1734489540328.gitgitgadget@gmail.com","subject":"[PATCH v5] git: use ^=1 to toggle between 0 and 1","fromName":"AreaZR via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-12-18T16:46:34Z","receivedAt":"2024-12-18T16:46:38Z","isPatch":true,"sender":{"key":"name:AreaZR","avatar":null},"body":"From: Seija Kijin \n\nIf it is known that an int is either 1 or 0,\ndoing an exclusive or to switch instead of a\nmodulus makes more sense and is more efficient.\n\nSigned-off-by: Seija Kijin \n---\n git: use ^=1 to toggle between 0 and 1\n \n If it is known that an int is either 1 or 0, doing an exclusive or to\n switch instead of a modulus makes more sense and is more efficient.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1620%2FAreaZR%2Fbuffer-v5\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1620/AreaZR/buffer-v5\nPull-Request: https://github.com/git/git/pull/1620\n\nRange-diff vs v4:\n\n 1: db1f0b1a323 ! 1: 5bb2cf10062 git: use ^=1 to toggle between 0 and 1\n @@ diff.c: static void mark_color_as_moved(struct diff_options *o,\n \t\t\telse\n \t\t\t\tflipped_block = 0;\n \n -\n - ## t/helper/test-path-utils.c ##\n -@@ t/helper/test-path-utils.c: static int check_dotfile(const char *x, const char **argv,\n - \tint res = 0, expect = 1;\n - \tfor (; *argv; argv++) {\n - \t\tif (!strcmp(\"--not\", *argv))\n --\t\t\texpect = !expect;\n -+\t\t\texpect ^= 1;\n - \t\telse if (expect != (is_hfs(*argv) || is_ntfs(*argv)))\n - \t\t\tres = error(\"'%s' is %s.git%s\", *argv,\n - \t\t\t\t expect ? \"not \" : \"\", x);\n\n\n diff.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 266ddf18e73..5c2ac8d6fd1 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1231,7 +1231,7 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\t\t\t\t\t\t &pmb_nr);\n \n \t\t\tif (contiguous && pmb_nr && moved_symbol == l->s)\n-\t\t\t\tflipped_block = (flipped_block + 1) % 2;\n+\t\t\t\tflipped_block ^= 1;\n \t\t\telse\n \t\t\t\tflipped_block = 0;\n \n\nbase-commit: d882f382b3d939d90cfa58d17b17802338f05d66\n-- \ngitgitgadget\n"},{"id":"509309","messageId":"pull.1620.v6.git.git.1734541037465.gitgitgadget@gmail.com","threadId":"60608","inReplyTo":"pull.1620.v5.git.git.1734540395021.gitgitgadget@gmail.com","subject":"[PATCH v6] git: use logical-not operator to toggle between 0 and 1","fromName":"AreaZR via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-12-18T16:57:17Z","receivedAt":"2024-12-18T16:57:20Z","isPatch":true,"sender":{"key":"name:AreaZR","avatar":null},"body":"From: Seija Kijin \n\nIf it is known that an int is either 1 or 0,\nusing a logical-not to switch instead of a\nmodulus makes more sense and is more efficient.\n\nSigned-off-by: Seija Kijin \n---\n git: use logical-not operator to toggle between 0 and 1\n \n If it is known that an int is either 1 or 0, doing an exclusive or to\n switch instead of a modulus makes more sense and is more efficient.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1620%2FAreaZR%2Fbuffer-v6\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1620/AreaZR/buffer-v6\nPull-Request: https://github.com/git/git/pull/1620\n\nRange-diff vs v5:\n\n 1: 5bb2cf10062 ! 1: 0fe9e776177 git: use ^=1 to toggle between 0 and 1\n @@ Metadata\n Author: Seija Kijin \n \n ## Commit message ##\n - git: use ^=1 to toggle between 0 and 1\n + git: use logical-not operator to toggle between 0 and 1\n \n If it is known that an int is either 1 or 0,\n - doing an exclusive or to switch instead of a\n + using a logical-not to switch instead of a\n modulus makes more sense and is more efficient.\n \n Signed-off-by: Seija Kijin \n @@ diff.c: static void mark_color_as_moved(struct diff_options *o,\n \n \t\t\tif (contiguous && pmb_nr && moved_symbol == l->s)\n -\t\t\t\tflipped_block = (flipped_block + 1) % 2;\n -+\t\t\t\tflipped_block ^= 1;\n ++\t\t\t\tflipped_block = !flipped_block;\n \t\t\telse\n \t\t\t\tflipped_block = 0;\n \n\n\n diff.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 266ddf18e73..48335971a4c 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1231,7 +1231,7 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\t\t\t\t\t\t &pmb_nr);\n \n \t\t\tif (contiguous && pmb_nr && moved_symbol == l->s)\n-\t\t\t\tflipped_block = (flipped_block + 1) % 2;\n+\t\t\t\tflipped_block = !flipped_block;\n \t\t\telse\n \t\t\t\tflipped_block = 0;\n \n\nbase-commit: d882f382b3d939d90cfa58d17b17802338f05d66\n-- \ngitgitgadget\n"},{"id":"509330","messageId":"xmqqttb0m04e.fsf@gitster.g","threadId":"60608","inReplyTo":"pull.1620.v6.git.git.1734541037465.gitgitgadget@gmail.com","subject":"Re: [PATCH v6] git: use logical-not operator to toggle between 0 and 1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-19T10:35:13Z","receivedAt":"2024-12-19T10:35:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"AreaZR via GitGitGadget\" writes:\n\n> From: Seija Kijin \n>\n> If it is known that an int is either 1 or 0,\n\nThis justification needs to be updated.\n\nThe point of using flip = !flip pattern is not that you do not\nassume \"1 or 0\", but follow \"zero or non-zero\", which is more\nidiomatic in C.\n\nOther than that, \n\n> -\t\t\t\tflipped_block = (flipped_block + 1) % 2;\n> -+\t\t\t\tflipped_block ^= 1;\n> ++\t\t\t\tflipped_block = !flipped_block;\n\nit indeed is a good change to avoid modulo-2 like this.\n\nThanks.\n\n"}]}