{"thread":{"id":"63906","subject":"[PATCH] revert: initialize const value","startedAt":"2025-08-04T13:00:12Z","lastAt":"2025-08-04T14:26:20Z","messageCount":5,"participants":["Jeff King","Patrick Steinhardt","Phillip Wood","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"523436","messageId":"20250804130011.GA93475@coredump.intra.peff.net","threadId":"63906","inReplyTo":null,"subject":"[PATCH] revert: initialize const value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-04T13:00:11Z","receivedAt":"2025-08-04T13:00:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When building with clang-22 and DEVELOPER=1 mode, this warning causes us\nto fail compilation:\n\n builtin/revert.c:114:13: error: default initialization of an object of type 'const char' leaves the object uninitialized [-Werror,-Wdefault-const-init-var-unsafe]\n 114 | const char sentinel_value;\n | ^\n\nThe compiler is right that this code is a bit funny. We declare a const\nvalue without an initializer. It cannot be assigned to because of the\nconst, but without an initializer it has no predictable value. So as a\nvariable it can never have any useful function, and if we tried to look\nat it, we'd get undefined behavior.\n\nBut it does have a function. We never use its value, but rather use its\naddress as a sentinel value for some other variables:\n\n const char *gpg_sign = &sentinel_value;\n\n\t...maybe set gpg_sign via parse_options...\n\n\tif (gpg_sign != &sentinel_value)\n\t\t...we got a non-default value...\n\nNormally we'd use NULL as a sentinel value for a pointer, but it doesn't\nwork here because we also want to detect --no-gpg-sign, which is marked\nby setting the pointer to NULL. We need a separate \"this was not\ntouched\" value, which is what this sentinel variable gives us.\n\nSo the code is correct as-is, but the sentinel variable itself is funny\nenough that it's understandable for a compiler warning to flag it. Let's\ntry to appease the compiler.\n\nThere are a few possible options:\n\n 1. Instead of a variable, we could just construct an artificial\n sentinel address like \"1\", \"-1\", etc. I think these technically\n fall afoul of the C standard (even if we do not access them, even\n constructing invalid pointers is not always allowed). But it's also\n something we do elsewhere, and even happens in some standard\n interfaces (e.g., mmap()'s MMAP_FAILED value). It does involve some\n annoying casts, though.\n\n 2. We can mark it as static. That gives it a definite value, but\n perhaps makes people wonder if the static-ness is important, when\n it's not.\n\n 3. We can just give it a value to shut the compiler up, even though\n nobody cares about that value.\n\nI went with (3) here as the smallest and most obvious change.\n\nSigned-off-by: Jeff King \n---\nI dunno, maybe the comment just makes things more mysterious and\ndoing the casts would make it more clear what is going on.\n\n builtin/revert.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex e07c2217fe..c3f92b585d 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -111,7 +111,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,\n \tconst char * const * usage_str = revert_or_cherry_pick_usage(opts);\n \tconst char *me = action_name(opts);\n \tconst char *cleanup_arg = NULL;\n-\tconst char sentinel_value;\n+\tconst char sentinel_value = 0; /* value not important */\n \tconst char *strategy = &sentinel_value;\n \tconst char *gpg_sign = &sentinel_value;\n \tenum empty_action empty_opt = EMPTY_COMMIT_UNSPECIFIED;\n-- \n2.50.1.786.g492fc26cdf\n"},{"id":"523437","messageId":"20250804130141.GA95101@coredump.intra.peff.net","threadId":"63906","inReplyTo":"20250804130011.GA93475@coredump.intra.peff.net","subject":"Re: [PATCH] revert: initialize const value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-04T13:01:41Z","receivedAt":"2025-08-04T13:01:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 04, 2025 at 09:00:12AM -0400, Jeff King wrote:\n\n> There are a few possible options:\n> \n> 1. Instead of a variable, we could just construct an artificial\n> sentinel address like \"1\", \"-1\", etc. I think these technically\n> fall afoul of the C standard (even if we do not access them, even\n> constructing invalid pointers is not always allowed). But it's also\n> something we do elsewhere, and even happens in some standard\n> interfaces (e.g., mmap()'s MMAP_FAILED value). It does involve some\n> annoying casts, though.\n> \n> 2. We can mark it as static. That gives it a definite value, but\n> perhaps makes people wonder if the static-ness is important, when\n> it's not.\n> \n> 3. We can just give it a value to shut the compiler up, even though\n> nobody cares about that value.\n> \n> I went with (3) here as the smallest and most obvious change.\n> \n> Signed-off-by: Jeff King \n> ---\n> I dunno, maybe the comment just makes things more mysterious and\n> doing the casts would make it more clear what is going on.\n\nHmm, I guess one other option I did not consider: we could just drop the\n\"const\". The pointers to it are \"const char *\", but it is fine for them\nto point to a non-const variable. Maybe that is less mysterious.\n\n-Peff\n"},{"id":"523439","messageId":"aJC3OiJoqJRnR_My@pks.im","threadId":"63906","inReplyTo":"20250804130141.GA95101@coredump.intra.peff.net","subject":"Re: [PATCH] revert: initialize const value","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-04T13:35:54Z","receivedAt":"2025-08-04T13:36:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Aug 04, 2025 at 09:01:41AM -0400, Jeff King wrote:\n> On Mon, Aug 04, 2025 at 09:00:12AM -0400, Jeff King wrote:\n> \n> > There are a few possible options:\n> > \n> > 1. Instead of a variable, we could just construct an artificial\n> > sentinel address like \"1\", \"-1\", etc. I think these technically\n> > fall afoul of the C standard (even if we do not access them, even\n> > constructing invalid pointers is not always allowed). But it's also\n> > something we do elsewhere, and even happens in some standard\n> > interfaces (e.g., mmap()'s MMAP_FAILED value). It does involve some\n> > annoying casts, though.\n> > \n> > 2. We can mark it as static. That gives it a definite value, but\n> > perhaps makes people wonder if the static-ness is important, when\n> > it's not.\n> > \n> > 3. We can just give it a value to shut the compiler up, even though\n> > nobody cares about that value.\n> > \n> > I went with (3) here as the smallest and most obvious change.\n> > \n> > Signed-off-by: Jeff King \n> > ---\n> > I dunno, maybe the comment just makes things more mysterious and\n> > doing the casts would make it more clear what is going on.\n> \n> Hmm, I guess one other option I did not consider: we could just drop the\n> \"const\". The pointers to it are \"const char *\", but it is fine for them\n> to point to a non-const variable. Maybe that is less mysterious.\n\nInitializing the value feels like a pragmatic choice to me. There is no\ndownside, and anyone who might be puzzled by the comment is likely to\ngit-blame(1) to your commit anyway. So I think the current version is\ngood enough.\n\nThanks!\n\nPatrick\n"},{"id":"523443","messageId":"ebcc5e06-6d0f-43f0-951a-226558702a9e@gmail.com","threadId":"63906","inReplyTo":"aJC3OiJoqJRnR_My@pks.im","subject":"Re: [PATCH] revert: initialize const value","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-08-04T13:51:35Z","receivedAt":"2025-08-04T13:51:41Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 04/08/2025 14:35, Patrick Steinhardt wrote:\n> On Mon, Aug 04, 2025 at 09:01:41AM -0400, Jeff King wrote:\n>> \n>> Hmm, I guess one other option I did not consider: we could just drop the\n>> \"const\". The pointers to it are \"const char *\", but it is fine for them\n>> to point to a non-const variable. Maybe that is less mysterious.\n> \n> Initializing the value feels like a pragmatic choice to me. There is no\n> downside, and anyone who might be puzzled by the comment is likely to\n> git-blame(1) to your commit anyway. So I think the current version is\n> good enough.\n\nYes the current version looks reasonable to me\n\nThanks\n\nPhillip\n\n"},{"id":"523447","messageId":"xmqqy0rz88ol.fsf@gitster.g","threadId":"63906","inReplyTo":"aJC3OiJoqJRnR_My@pks.im","subject":"Re: [PATCH] revert: initialize const value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-04T14:26:18Z","receivedAt":"2025-08-04T14:26:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Initializing the value feels like a pragmatic choice to me. There is no\n> downside, and anyone who might be puzzled by the comment is likely to\n> git-blame(1) to your commit anyway. So I think the current version is\n> good enough.\n\nI had to run \"git blame\" to find out who was being overly clever by\nusing a sentinel like this. I am glad that it was not me ;-)\n\nThe fix looks good. It may make it even better if we renamed the\nvariable to sentinel_address or something ;-).\n"}]}