{"thread":{"id":"60216","subject":"[PATCH 1/2] parse-options: add int value pointer to struct option","startedAt":"2023-09-09T21:10:52Z","lastAt":"2023-10-03T18:24:43Z","messageCount":36,"participants":["René Scharfe","Oswald Buddenhagen","Taylor Blau","Junio C Hamano","Jeff King","Kristoffer Haugsbakk","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"481631","messageId":"2d6f3d74-687a-2d40-5c0c-abc396aef80f@web.de","threadId":"60216","inReplyTo":null,"subject":"[PATCH 1/2] parse-options: add int value pointer to struct option","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-09-09T21:10:36Z","receivedAt":"2023-09-09T21:10:52Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add an int pointer, value_int, to struct option to provide a typed value\npointer for the various integer options. It allows type checks at\ncompile time, which is not possible with the void pointer, value. Its\nuse is optional for now.\n\nSigned-off-by: René Scharfe \n---\n parse-options.c | 34 +++++++++++++++++++---------------\n parse-options.h | 2 ++\n 2 files changed, 21 insertions(+), 15 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex e8e076c3a6..2552745804 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -82,10 +82,11 @@ static enum parse_opt_result opt_command_mode_error(\n \t * already, and report that this is not compatible with it.\n \t */\n \tfor (that = all_opts; that->type != OPTION_END; that++) {\n+\t\tint *value_int = opt->value_int ? opt->value_int : opt->value;\n \t\tif (that == opt ||\n \t\t !(that->flags & PARSE_OPT_CMDMODE) ||\n \t\t that->value != opt->value ||\n-\t\t that->defval != *(int *)opt->value)\n+\t\t that->defval != *value_int)\n \t\t\tcontinue;\n\n \t\tif (that->long_name)\n@@ -109,6 +110,7 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n \tconst char *s, *arg;\n \tconst int unset = flags & OPT_UNSET;\n \tint err;\n+\tint *value_int = opt->value_int ? opt->value_int : opt->value;\n\n \tif (unset && p->opt)\n \t\treturn error(_(\"%s takes no value\"), optname(opt, flags));\n@@ -122,7 +124,7 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n \t * is not a grave error, so let it pass.\n \t */\n \tif ((opt->flags & PARSE_OPT_CMDMODE) &&\n-\t *(int *)opt->value && *(int *)opt->value != opt->defval)\n+\t *value_int && *value_int != opt->defval)\n \t\treturn opt_command_mode_error(opt, all_opts, flags);\n\n \tswitch (opt->type) {\n@@ -131,33 +133,33 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n\n \tcase OPTION_BIT:\n \t\tif (unset)\n-\t\t\t*(int *)opt->value &= ~opt->defval;\n+\t\t\t*value_int &= ~opt->defval;\n \t\telse\n-\t\t\t*(int *)opt->value |= opt->defval;\n+\t\t\t*value_int |= opt->defval;\n \t\treturn 0;\n\n \tcase OPTION_NEGBIT:\n \t\tif (unset)\n-\t\t\t*(int *)opt->value |= opt->defval;\n+\t\t\t*value_int |= opt->defval;\n \t\telse\n-\t\t\t*(int *)opt->value &= ~opt->defval;\n+\t\t\t*value_int &= ~opt->defval;\n \t\treturn 0;\n\n \tcase OPTION_BITOP:\n \t\tif (unset)\n \t\t\tBUG(\"BITOP can't have unset form\");\n-\t\t*(int *)opt->value &= ~opt->extra;\n-\t\t*(int *)opt->value |= opt->defval;\n+\t\t*value_int &= ~opt->extra;\n+\t\t*value_int |= opt->defval;\n \t\treturn 0;\n\n \tcase OPTION_COUNTUP:\n-\t\tif (*(int *)opt->value < 0)\n-\t\t\t*(int *)opt->value = 0;\n-\t\t*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;\n+\t\tif (*value_int < 0)\n+\t\t\t*value_int = 0;\n+\t\t*value_int = unset ? 0 : *value_int + 1;\n \t\treturn 0;\n\n \tcase OPTION_SET_INT:\n-\t\t*(int *)opt->value = unset ? 0 : opt->defval;\n+\t\t*value_int = unset ? 0 : opt->defval;\n \t\treturn 0;\n\n \tcase OPTION_STRING:\n@@ -206,11 +208,11 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n \t}\n \tcase OPTION_INTEGER:\n \t\tif (unset) {\n-\t\t\t*(int *)opt->value = 0;\n+\t\t\t*value_int = 0;\n \t\t\treturn 0;\n \t\t}\n \t\tif (opt->flags & PARSE_OPT_OPTARG && !p->opt) {\n-\t\t\t*(int *)opt->value = opt->defval;\n+\t\t\t*value_int = opt->defval;\n \t\t\treturn 0;\n \t\t}\n \t\tif (get_arg(p, opt, flags, &arg))\n@@ -218,7 +220,7 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n \t\tif (!*arg)\n \t\t\treturn error(_(\"%s expects a numerical value\"),\n \t\t\t\t optname(opt, flags));\n-\t\t*(int *)opt->value = strtol(arg, (char **)&s, 10);\n+\t\t*value_int = strtol(arg, (char **)&s, 10);\n \t\tif (*s)\n \t\t\treturn error(_(\"%s expects a numerical value\"),\n \t\t\t\t optname(opt, flags));\n@@ -483,6 +485,8 @@ static void parse_options_check(const struct option *opts)\n \t\tif (opts->type == OPTION_SET_INT && !opts->defval &&\n \t\t opts->long_name && !(opts->flags & PARSE_OPT_NONEG))\n \t\t\toptbug(opts, \"OPTION_SET_INT 0 should not be negatable\");\n+\t\tif (opts->value && opts->value_int)\n+\t\t\toptbug(opts, \"only a single value type supported\");\n \t\tswitch (opts->type) {\n \t\tcase OPTION_COUNTUP:\n \t\tcase OPTION_BIT:\ndiff --git a/parse-options.h b/parse-options.h\nindex 57a7fe9d91..5e7475bd2d 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -158,6 +158,8 @@ struct option {\n \tparse_opt_ll_cb *ll_callback;\n \tintptr_t extra;\n \tparse_opt_subcommand_fn *subcommand_fn;\n+\n+\tint *value_int;\n };\n\n #define OPT_BIT_F(s, l, v, h, b, f) { \\\n--\n2.42.0\n"},{"id":"481632","messageId":"e6d8a291-03de-cfd3-3813-747fc2cad145@web.de","threadId":"60216","inReplyTo":"2d6f3d74-687a-2d40-5c0c-abc396aef80f@web.de","subject":"[PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-09-09T21:14:20Z","receivedAt":"2023-09-09T21:14:31Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Some uses of OPT_CMDMODE provide a pointer to an enum. It is\ndereferenced as an int pointer in parse-options.c::get_value(). These\ntwo types are incompatible, though -- the storage size of an enum can\nvary between platforms. C23 would allow us to specify the underlying\ntype of the different enums, making them compatible, but with C99 the\neasiest safe option is to actually use int as the value type.\n\nConvert the offending OPT_CMDMODE users and use the typed value_int\npoint in the macro's definition to enforce that type for future ones.\n\nSigned-off-by: René Scharfe \n---\n builtin/am.c | 2 +-\n builtin/help.c | 5 +++--\n builtin/ls-tree.c | 2 +-\n builtin/rebase.c | 2 +-\n builtin/replace.c | 3 ++-\n builtin/stripspace.c | 2 +-\n parse-options.h | 2 +-\n 7 files changed, 10 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 202040b62e..ebb72ebaaa 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -2269,7 +2269,7 @@ enum resume_type {\n };\n\n struct resume_mode {\n-\tenum resume_type mode;\n+\tint mode;\n \tenum show_patch_type sub_mode;\n };\n\ndiff --git a/builtin/help.c b/builtin/help.c\nindex dc1fbe2b98..e8aedb932c 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -42,7 +42,7 @@ enum show_config_type {\n \tSHOW_CONFIG_SECTIONS,\n };\n\n-static enum help_action {\n+enum help_action {\n \tHELP_ACTION_ALL = 1,\n \tHELP_ACTION_GUIDES,\n \tHELP_ACTION_CONFIG,\n@@ -50,7 +50,8 @@ static enum help_action {\n \tHELP_ACTION_DEVELOPER_INTERFACES,\n \tHELP_ACTION_CONFIG_FOR_COMPLETION,\n \tHELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION,\n-} cmd_mode;\n+};\n+static int cmd_mode;\n\n static const char *html_path;\n static int verbose = 1;\ndiff --git a/builtin/ls-tree.c b/builtin/ls-tree.c\nindex 209d2dc0d5..6f8c43f729 100644\n--- a/builtin/ls-tree.c\n+++ b/builtin/ls-tree.c\n@@ -346,7 +346,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n \tint i, full_tree = 0;\n \tint full_name = !prefix || !*prefix;\n \tread_tree_fn_t fn = NULL;\n-\tenum ls_tree_cmdmode cmdmode = MODE_DEFAULT;\n+\tint cmdmode = MODE_DEFAULT;\n \tint null_termination = 0;\n \tstruct ls_tree_options options = { 0 };\n \tconst struct option ls_tree_options[] = {\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 50cb85751f..d11e749579 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -111,7 +111,7 @@ struct rebase_options {\n \t\tREBASE_INTERACTIVE_EXPLICIT = 1<<4,\n \t} flags;\n \tstruct strvec git_am_opts;\n-\tenum action action;\n+\tint action;\n \tchar *reflog_action;\n \tint signoff;\n \tint allow_rerere_autoupdate;\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex da59600ad2..d0063d3feb 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -553,7 +553,8 @@ int cmd_replace(int argc, const char **argv, const char *prefix)\n \t\tMODE_GRAFT,\n \t\tMODE_CONVERT_GRAFT_FILE,\n \t\tMODE_REPLACE\n-\t} cmdmode = MODE_UNSPECIFIED;\n+\t};\n+\tint cmdmode = MODE_UNSPECIFIED;\n \tstruct option options[] = {\n \t\tOPT_CMDMODE('l', \"list\", &cmdmode, N_(\"list replace refs\"), MODE_LIST),\n \t\tOPT_CMDMODE('d', \"delete\", &cmdmode, N_(\"delete replace refs\"), MODE_DELETE),\ndiff --git a/builtin/stripspace.c b/builtin/stripspace.c\nindex 7b700a9fb1..f6de0b17dc 100644\n--- a/builtin/stripspace.c\n+++ b/builtin/stripspace.c\n@@ -32,7 +32,7 @@ enum stripspace_mode {\n int cmd_stripspace(int argc, const char **argv, const char *prefix)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tenum stripspace_mode mode = STRIP_DEFAULT;\n+\tint mode = STRIP_DEFAULT;\n \tint nongit;\n\n \tconst struct option options[] = {\ndiff --git a/parse-options.h b/parse-options.h\nindex 5e7475bd2d..349c3fca04 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -262,7 +262,7 @@ struct option {\n \t.type = OPTION_SET_INT, \\\n \t.short_name = (s), \\\n \t.long_name = (l), \\\n-\t.value = (v), \\\n+\t.value_int = (v), \\\n \t.help = (h), \\\n \t.flags = PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG | (f), \\\n \t.defval = (i), \\\n--\n2.42.0\n"},{"id":"481650","messageId":"ZP2X9roiaeEjzf24@ugly","threadId":"60216","inReplyTo":"e6d8a291-03de-cfd3-3813-747fc2cad145@web.de","subject":"Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-09-10T10:18:30Z","receivedAt":"2023-09-10T10:18:36Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Sat, Sep 09, 2023 at 11:14:20PM +0200, René Scharfe wrote:\n>Convert the offending OPT_CMDMODE users and use the typed value_int\n>point in the macro's definition to enforce that type for future ones.\n>\nthat defeats -Wswitch[-enum], though.\n\nthe pedantically correct solution would be using setter callbacks.\n\nregards\n"},{"id":"481657","messageId":"ZP4NrVeqMtFTLEuf@nand.local","threadId":"60216","inReplyTo":"2d6f3d74-687a-2d40-5c0c-abc396aef80f@web.de","subject":"Re: [PATCH 1/2] parse-options: add int value pointer to struct option","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-10T18:40:45Z","receivedAt":"2023-09-10T18:40:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Sep 09, 2023 at 11:10:36PM +0200, René Scharfe wrote:\n> Add an int pointer, value_int, to struct option to provide a typed value\n> pointer for the various integer options. It allows type checks at\n> compile time, which is not possible with the void pointer, value. Its\n> use is optional for now.\n\nThis is an interesting direction. I wonder about whether or not you'd\nconsider changing the option structure to contain a tagged union type\nthat represents some common cases we'd want from a parse-options\ncallback, something like:\n\n struct option {\n /* ... */\n union {\n void *value;\n int *value_int;\n /* etc ... */\n } u;\n enum option_type t;\n };\n\nwhere option_type has some value corresponding to \"void *\", another for\n\"int *\", and so on.\n\nAlternatively, perhaps you are thinking that we'd use both the value\npointer and the value_int pointer to point at potentially different\nvalues in the same callback. I don't have strong feelings about it, but\nI'd just as soon encourage us to shy away from that approach, since\nassigning a single callback parameter to each function seems more\norganized.\n\n> @@ -109,6 +110,7 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n> \tconst char *s, *arg;\n> \tconst int unset = flags & OPT_UNSET;\n> \tint err;\n> +\tint *value_int = opt->value_int ? opt->value_int : opt->value;\n>\n> \tif (unset && p->opt)\n> \t\treturn error(_(\"%s takes no value\"), optname(opt, flags));\n\nReading this hunk, I wonder whether we even need a type tag (the\noption_type enum above) if each callback knows a priori what type it\nexpects. But I think storing them together in a union makes sense to do.\n\nThanks,\nTaylor\n"},{"id":"481685","messageId":"15530a5f-8d06-24c9-bc2d-e313c895f477@web.de","threadId":"60216","inReplyTo":"ZP2X9roiaeEjzf24@ugly","subject":"Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-09-11T20:11:56Z","receivedAt":"2023-09-11T21:38:45Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 10.09.23 um 12:18 schrieb Oswald Buddenhagen:\n> On Sat, Sep 09, 2023 at 11:14:20PM +0200, René Scharfe wrote:\n>> Convert the offending OPT_CMDMODE users and use the typed value_int\n>> point in the macro's definition to enforce that type for future ones.\n>>\n> that defeats -Wswitch[-enum], though.\n\nTrue. Though I don't fully understand these warnings (why not then\nalso warn about if without else?), but taking them away is a bit rude\nto those who care.\n\n> the pedantically correct solution would be using setter callbacks.\n\nOr to use an int to point to and then copy into a companion enum\nvariable to after parsing, which would be my choice.\n\nRené\n"},{"id":"481699","messageId":"683efb6d-dc41-51ff-f048-7a23ee955e00@web.de","threadId":"60216","inReplyTo":"ZP4NrVeqMtFTLEuf@nand.local","subject":"Re: [PATCH 1/2] parse-options: add int value pointer to struct option","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-09-11T20:12:00Z","receivedAt":"2023-09-11T21:39:19Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 10.09.23 um 20:40 schrieb Taylor Blau:\n> On Sat, Sep 09, 2023 at 11:10:36PM +0200, René Scharfe wrote:\n>> Add an int pointer, value_int, to struct option to provide a typed value\n>> pointer for the various integer options. It allows type checks at\n>> compile time, which is not possible with the void pointer, value. Its\n>> use is optional for now.\n>\n> This is an interesting direction. I wonder about whether or not you'd\n> consider changing the option structure to contain a tagged union type\n> that represents some common cases we'd want from a parse-options\n> callback, something like:\n>\n> struct option {\n> /* ... */\n> union {\n> void *value;\n> int *value_int;\n> /* etc ... */\n> } u;\n> enum option_type t;\n> };\n>\n> where option_type has some value corresponding to \"void *\", another for\n> \"int *\", and so on.\n\nIn a hand-made struct option this would only provide a very limited form\nof type safety. It reduces the number of incorrect types to choose from\nfrom basically infinity to a handful, but still allows pointing the\nunion e.g. to an int for an option that takes a long or a string without\nany compiler warning or error.\n\nConvenience macros like OPT_CMDMODE could use the union to provide a\ntype safe interface, though, true. This might suffice for our purposes.\n\n> Alternatively, perhaps you are thinking that we'd use both the value\n> pointer and the value_int pointer to point at potentially different\n> values in the same callback. I don't have strong feelings about it, but\n> I'd just as soon encourage us to shy away from that approach, since\n> assigning a single callback parameter to each function seems more\n> organized.\n\nRight, we only need one active value pointer per option.\n\n>> @@ -109,6 +110,7 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n>> \tconst char *s, *arg;\n>> \tconst int unset = flags & OPT_UNSET;\n>> \tint err;\n>> +\tint *value_int = opt->value_int ? opt->value_int : opt->value;\n>>\n>> \tif (unset && p->opt)\n>> \t\treturn error(_(\"%s takes no value\"), optname(opt, flags));\n>\n> Reading this hunk, I wonder whether we even need a type tag (the\n> option_type enum above) if each callback knows a priori what type it\n> expects. But I think storing them together in a union makes sense to do.\n\nYes, option types (OPTION_INTEGER etc.) already imply a pointer type,\nno additional tag needed.\n\nRené\n"},{"id":"481705","messageId":"54475801-0387-468e-bd90-a5ea0d677bca@web.de","threadId":"60216","inReplyTo":"xmqqedj4v808.fsf@gitster.g","subject":"Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-09-11T20:11:52Z","receivedAt":"2023-09-11T21:39:23Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"\nAm 11.09.23 um 21:12 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>> Some uses of OPT_CMDMODE provide a pointer to an enum. It is\n>> dereferenced as an int pointer in parse-options.c::get_value(). These\n>> two types are incompatible, though -- the storage size of an enum can\n>> vary between platforms. C23 would allow us to specify the underlying\n>> type of the different enums, making them compatible, but with C99 the\n>> easiest safe option is to actually use int as the value type.\n>>\n>> Convert the offending OPT_CMDMODE users and use the typed value_int\n>> point in the macro's definition to enforce that type for future ones.\n>\n> Interesting. I wondered if this means that applying [1/2] alone\n> will immediately break these places that [2/2] fixes, but the answer\n> is no, as the previous step did not make these places use the typed\n> pointer. But it also means that with this step alone to use \"int\",\n> instead of various \"enum\" types that can have representations that\n> are different from \"int\", would already \"fix\" the current code\n> while still casing back and forth from (void *)?\n\nYes and yes. And the change to use value_int on its own makes the type\nmismatch visible via compiler warnings. It guards against future\nviolations.\n\nRené\n"},{"id":"481713","messageId":"xmqqedj4v808.fsf@gitster.g","threadId":"60216","inReplyTo":"e6d8a291-03de-cfd3-3813-747fc2cad145@web.de","subject":"Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-11T19:12:55Z","receivedAt":"2023-09-11T23:02:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Some uses of OPT_CMDMODE provide a pointer to an enum. It is\n> dereferenced as an int pointer in parse-options.c::get_value(). These\n> two types are incompatible, though -- the storage size of an enum can\n> vary between platforms. C23 would allow us to specify the underlying\n> type of the different enums, making them compatible, but with C99 the\n> easiest safe option is to actually use int as the value type.\n>\n> Convert the offending OPT_CMDMODE users and use the typed value_int\n> point in the macro's definition to enforce that type for future ones.\n\nInteresting. I wondered if this means that applying [1/2] alone\nwill immediately break these places that [2/2] fixes, but the answer\nis no, as the previous step did not make these places use the typed\npointer. But it also means that with this step alone to use \"int\",\ninstead of various \"enum\" types that can have representations that\nare different from \"int\", would already \"fix\" the current code\nwhile still casing back and forth from (void *)?\n\nIn any case, the two-patch series looks good, and it does not break\nbisectability, either.\n\nThanks.\n"},{"id":"481715","messageId":"xmqq7cowv7pm.fsf@gitster.g","threadId":"60216","inReplyTo":"ZP4NrVeqMtFTLEuf@nand.local","subject":"Re: [PATCH 1/2] parse-options: add int value pointer to struct option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-11T19:19:17Z","receivedAt":"2023-09-11T23:02:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> callback, something like:\n>\n> struct option {\n> /* ... */\n> union {\n> void *value;\n> int *value_int;\n> /* etc ... */\n> } u;\n> enum option_type t;\n> };\n>\n> where option_type has some value corresponding to \"void *\", another for\n> \"int *\", and so on.\n\nYup, that does cross my mind, even though I would have used\n\n\tunion {\n\t\tvoid *void_ptr;\n\t\tint *int_ptr;\n\t} value;\n\nor something without a rather meaningless 'u'.\n\n> Alternatively, perhaps you are thinking that we'd use both the value\n> pointer and the value_int pointer to point at potentially different\n> values in the same callback. I don't have strong feelings about it, but\n> I'd just as soon encourage us to shy away from that approach, since\n> assigning a single callback parameter to each function seems more\n> organized.\n\nWe have seen (with Peff's \"-Wunused\" work) that there are small\nnumber of cases that it would be handy for a callback to be told the\nlocations of multiple external variables, but I do not think it\nwould be a good solution to that problem to have \"void *value\" and\n\"int value_int\" next to each other and allow them to coexist, as it\nwould work only when these multiple variables happen to be of the\nright types.\n"},{"id":"481730","messageId":"ZP+UgvIon1lrIFa+@ugly","threadId":"60216","inReplyTo":"xmqq7cowv7pm.fsf@gitster.g","subject":"Re: [PATCH 1/2] parse-options: add int value pointer to struct option","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-09-11T22:28:18Z","receivedAt":"2023-09-12T02:08:06Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Mon, Sep 11, 2023 at 12:19:17PM -0700, Junio C Hamano wrote:\n>Yup, that does cross my mind, even though I would have used\n>\n>\tunion {\n>\t\tvoid *void_ptr;\n>\t\tint *int_ptr;\n>\t} value;\n>\n>or something without a rather meaningless 'u'.\n>\ni'd go the opposite way and make it an anonymous union.\nthat would require c11, though. imo not exactly an outrageous \nproposition in 2023, but ...\n\nregards\n"},{"id":"481750","messageId":"20230912084029.GD1630538@coredump.intra.peff.net","threadId":"60216","inReplyTo":"15530a5f-8d06-24c9-bc2d-e313c895f477@web.de","subject":"Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-12T08:40:29Z","receivedAt":"2023-09-12T08:40:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 11, 2023 at 10:11:56PM +0200, René Scharfe wrote:\n\n> Am 10.09.23 um 12:18 schrieb Oswald Buddenhagen:\n> > On Sat, Sep 09, 2023 at 11:14:20PM +0200, René Scharfe wrote:\n> >> Convert the offending OPT_CMDMODE users and use the typed value_int\n> >> point in the macro's definition to enforce that type for future ones.\n> >>\n> > that defeats -Wswitch[-enum], though.\n> \n> True. Though I don't fully understand these warnings (why not then\n> also warn about if without else?), but taking them away is a bit rude\n> to those who care.\n\nI think losing warnings is unfortunate, but it's just one example.\nWe're losing the type information completely from the values. That might\nbe of use to the compiler (both for -Wswitch, but also for code\ngeneration in general). But it is also of use to human readers, who see\nthat \"foo\" is of type \"enum bar\" and know what it's supposed to contain.\n\n> > the pedantically correct solution would be using setter callbacks.\n> \n> Or to use an int to point to and then copy into a companion enum\n> variable to after parsing, which would be my choice.\n\nYeah, I had the same thought. I'm just not sure how to do that in a way\nthat isn't a pain for the callers.\n\n-Peff\n"},{"id":"481932","messageId":"xmqqa5tmau6e.fsf@gitster.g","threadId":"60216","inReplyTo":"20230912084029.GD1630538@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-16T17:45:29Z","receivedAt":"2023-09-16T17:46:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> True. Though I don't fully understand these warnings (why not then\n>> also warn about if without else?), but taking them away is a bit rude\n>> to those who care.\n>\n> I think losing warnings is unfortunate, but it's just one example.\n> We're losing the type information completely from the values.\n> ...\n>> Or to use an int to point to and then copy into a companion enum\n>> variable to after parsing, which would be my choice.\n>\n> Yeah, I had the same thought. I'm just not sure how to do that in a way\n> that isn't a pain for the callers.\n\nThe discussion seems to have petered out around this point.\nWhat (if anything) do we want to do with this topic?\n"},{"id":"481944","messageId":"6dc558c6-f78c-4d9c-8444-498de8e4d22a@web.de","threadId":"60216","inReplyTo":"xmqqa5tmau6e.fsf@gitster.g","subject":"Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-09-18T09:28:31Z","receivedAt":"2023-09-18T09:29:49Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 16.09.23 um 19:45 schrieb Junio C Hamano:\n> Jeff King writes:\n>\n>>> True. Though I don't fully understand these warnings (why not then\n>>> also warn about if without else?), but taking them away is a bit rude\n>>> to those who care.\n>>\n>> I think losing warnings is unfortunate, but it's just one example.\n>> We're losing the type information completely from the values.\n>> ...\n>>> Or to use an int to point to and then copy into a companion enum\n>>> variable to after parsing, which would be my choice.\n>>\n>> Yeah, I had the same thought. I'm just not sure how to do that in a way\n>> that isn't a pain for the callers.\n>\n> The discussion seems to have petered out around this point.\n> What (if anything) do we want to do with this topic?\n\nHere's a version that preserves the enums by using additional int\nvariables just for the parsing phase. No tricks. The diff is long, but\nmost changes aren't particularly complicated and the resulting code is\nnot that ugly. Except for builtin/am.c perhaps, which changes the\ncommand mode value using a callback as well.\n\n--- >8 ---\nSubject: [PATCH v2 2/2] parse-options: use and require int pointer for OPT_CMDMODE\n\nSome uses of OPT_CMDMODE provide a pointer to an enum. It is\ndereferenced as an int pointer in parse-options.c::get_value(). The two\ntypes are incompatible, though -- the storage size of an enum can vary\nbetween platforms. C23 would allow us to specify the underlying type of\nthe diffenrent enums, making them compatible, but with C99 the easiest\nsafe option is to actually use int as the value type.\n\nConvert the offending OPT_CMDMODE users to point to a new int value and\nset the enum value after parsing. Switch to the typed value_int pointer\nin the macro's definition to enforce that type for future users.\n\nSigned-off-by: René Scharfe \n---\n builtin/am.c | 24 +++++++++++++-----------\n builtin/help.c | 16 +++++++++-------\n builtin/ls-tree.c | 12 +++++++-----\n builtin/rebase.c | 15 +++++++++------\n builtin/replace.c | 12 +++++++-----\n builtin/stripspace.c | 6 ++++--\n parse-options.h | 2 +-\n 7 files changed, 50 insertions(+), 37 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 202040b62e..00930e2152 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -2270,13 +2270,14 @@ enum resume_type {\n\n struct resume_mode {\n \tenum resume_type mode;\n+\tint mode_int;\n \tenum show_patch_type sub_mode;\n };\n\n static int parse_opt_show_current_patch(const struct option *opt, const char *arg, int unset)\n {\n \tint *opt_value = opt->value;\n-\tstruct resume_mode *resume = container_of(opt_value, struct resume_mode, mode);\n+\tstruct resume_mode *resume = container_of(opt_value, struct resume_mode, mode_int);\n\n \t/*\n \t * Please update $__git_showcurrentpatch in git-completion.bash\n@@ -2300,12 +2301,12 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar\n \t\t\t\t \"--show-current-patch\", arg);\n \t}\n\n-\tif (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)\n+\tif (resume->mode_int == RESUME_SHOW_PATCH && new_value != resume->sub_mode)\n \t\treturn error(_(\"options '%s=%s' and '%s=%s' \"\n \t\t\t\t\t \"cannot be used together\"),\n \t\t\t\t\t \"--show-current-patch\", \"--show-current-patch\", arg, valid_modes[resume->sub_mode]);\n\n-\tresume->mode = RESUME_SHOW_PATCH;\n+\tresume->mode_int = RESUME_SHOW_PATCH;\n \tresume->sub_mode = new_value;\n \treturn 0;\n }\n@@ -2316,7 +2317,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \tint binary = -1;\n \tint keep_cr = -1;\n \tint patch_format = PATCH_FORMAT_UNKNOWN;\n-\tstruct resume_mode resume = { .mode = RESUME_FALSE };\n+\tstruct resume_mode resume = { .mode_int = RESUME_FALSE };\n \tint in_progress;\n \tint ret = 0;\n\n@@ -2387,27 +2388,27 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\t\tPARSE_OPT_NOARG),\n \t\tOPT_STRING(0, \"resolvemsg\", &state.resolvemsg, NULL,\n \t\t\tN_(\"override error message when patch failure occurs\")),\n-\t\tOPT_CMDMODE(0, \"continue\", &resume.mode,\n+\t\tOPT_CMDMODE(0, \"continue\", &resume.mode_int,\n \t\t\tN_(\"continue applying patches after resolving a conflict\"),\n \t\t\tRESUME_RESOLVED),\n-\t\tOPT_CMDMODE('r', \"resolved\", &resume.mode,\n+\t\tOPT_CMDMODE('r', \"resolved\", &resume.mode_int,\n \t\t\tN_(\"synonyms for --continue\"),\n \t\t\tRESUME_RESOLVED),\n-\t\tOPT_CMDMODE(0, \"skip\", &resume.mode,\n+\t\tOPT_CMDMODE(0, \"skip\", &resume.mode_int,\n \t\t\tN_(\"skip the current patch\"),\n \t\t\tRESUME_SKIP),\n-\t\tOPT_CMDMODE(0, \"abort\", &resume.mode,\n+\t\tOPT_CMDMODE(0, \"abort\", &resume.mode_int,\n \t\t\tN_(\"restore the original branch and abort the patching operation\"),\n \t\t\tRESUME_ABORT),\n-\t\tOPT_CMDMODE(0, \"quit\", &resume.mode,\n+\t\tOPT_CMDMODE(0, \"quit\", &resume.mode_int,\n \t\t\tN_(\"abort the patching operation but keep HEAD where it is\"),\n \t\t\tRESUME_QUIT),\n-\t\t{ OPTION_CALLBACK, 0, \"show-current-patch\", &resume.mode,\n+\t\t{ OPTION_CALLBACK, 0, \"show-current-patch\", &resume.mode_int,\n \t\t \"(diff|raw)\",\n \t\t N_(\"show the patch being applied\"),\n \t\t PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,\n \t\t parse_opt_show_current_patch, RESUME_SHOW_PATCH },\n-\t\tOPT_CMDMODE(0, \"allow-empty\", &resume.mode,\n+\t\tOPT_CMDMODE(0, \"allow-empty\", &resume.mode_int,\n \t\t\tN_(\"record the empty patch as an empty commit\"),\n \t\t\tRESUME_ALLOW_EMPTY),\n \t\tOPT_BOOL(0, \"committer-date-is-author-date\",\n@@ -2439,6 +2440,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\tam_load(&state);\n\n \targc = parse_options(argc, argv, prefix, options, usage, 0);\n+\tresume.mode = resume.mode_int;\n\n \tif (binary >= 0)\n \t\tfprintf_ln(stderr, _(\"The -b/--binary option has been a no-op for long time, and\\n\"\ndiff --git a/builtin/help.c b/builtin/help.c\nindex dc1fbe2b98..d76f88d544 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -51,6 +51,7 @@ static enum help_action {\n \tHELP_ACTION_CONFIG_FOR_COMPLETION,\n \tHELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION,\n } cmd_mode;\n+static int cmd_mode_int;\n\n static const char *html_path;\n static int verbose = 1;\n@@ -59,7 +60,7 @@ static int exclude_guides;\n static int show_external_commands = -1;\n static int show_aliases = -1;\n static struct option builtin_help_options[] = {\n-\tOPT_CMDMODE('a', \"all\", &cmd_mode, N_(\"print all available commands\"),\n+\tOPT_CMDMODE('a', \"all\", &cmd_mode_int, N_(\"print all available commands\"),\n \t\t HELP_ACTION_ALL),\n \tOPT_BOOL(0, \"external-commands\", &show_external_commands,\n \t\t N_(\"show external commands in --all\")),\n@@ -72,19 +73,19 @@ static struct option builtin_help_options[] = {\n \t\t\tHELP_FORMAT_INFO),\n \tOPT__VERBOSE(&verbose, N_(\"print command description\")),\n\n-\tOPT_CMDMODE('g', \"guides\", &cmd_mode, N_(\"print list of useful guides\"),\n+\tOPT_CMDMODE('g', \"guides\", &cmd_mode_int, N_(\"print list of useful guides\"),\n \t\t HELP_ACTION_GUIDES),\n-\tOPT_CMDMODE(0, \"user-interfaces\", &cmd_mode,\n+\tOPT_CMDMODE(0, \"user-interfaces\", &cmd_mode_int,\n \t\t N_(\"print list of user-facing repository, command and file interfaces\"),\n \t\t HELP_ACTION_USER_INTERFACES),\n-\tOPT_CMDMODE(0, \"developer-interfaces\", &cmd_mode,\n+\tOPT_CMDMODE(0, \"developer-interfaces\", &cmd_mode_int,\n \t\t N_(\"print list of file formats, protocols and other developer interfaces\"),\n \t\t HELP_ACTION_DEVELOPER_INTERFACES),\n-\tOPT_CMDMODE('c', \"config\", &cmd_mode, N_(\"print all configuration variable names\"),\n+\tOPT_CMDMODE('c', \"config\", &cmd_mode_int, N_(\"print all configuration variable names\"),\n \t\t HELP_ACTION_CONFIG),\n-\tOPT_CMDMODE_F(0, \"config-for-completion\", &cmd_mode, \"\",\n+\tOPT_CMDMODE_F(0, \"config-for-completion\", &cmd_mode_int, \"\",\n \t\t HELP_ACTION_CONFIG_FOR_COMPLETION, PARSE_OPT_HIDDEN),\n-\tOPT_CMDMODE_F(0, \"config-sections-for-completion\", &cmd_mode, \"\",\n+\tOPT_CMDMODE_F(0, \"config-sections-for-completion\", &cmd_mode_int, \"\",\n \t\t HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION, PARSE_OPT_HIDDEN),\n\n \tOPT_END(),\n@@ -640,6 +641,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, builtin_help_options,\n \t\t\tbuiltin_help_usage, 0);\n \tparsed_help_format = help_format;\n+\tcmd_mode = cmd_mode_int;\n\n \tif (cmd_mode != HELP_ACTION_ALL &&\n \t (show_external_commands >= 0 ||\ndiff --git a/builtin/ls-tree.c b/builtin/ls-tree.c\nindex 209d2dc0d5..c64b38614a 100644\n--- a/builtin/ls-tree.c\n+++ b/builtin/ls-tree.c\n@@ -346,7 +346,8 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n \tint i, full_tree = 0;\n \tint full_name = !prefix || !*prefix;\n \tread_tree_fn_t fn = NULL;\n-\tenum ls_tree_cmdmode cmdmode = MODE_DEFAULT;\n+\tenum ls_tree_cmdmode cmdmode;\n+\tint cmdmode_int = MODE_DEFAULT;\n \tint null_termination = 0;\n \tstruct ls_tree_options options = { 0 };\n \tconst struct option ls_tree_options[] = {\n@@ -358,13 +359,13 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n \t\t\tLS_SHOW_TREES),\n \t\tOPT_BOOL('z', NULL, &null_termination,\n \t\t\t N_(\"terminate entries with NUL byte\")),\n-\t\tOPT_CMDMODE('l', \"long\", &cmdmode, N_(\"include object size\"),\n+\t\tOPT_CMDMODE('l', \"long\", &cmdmode_int, N_(\"include object size\"),\n \t\t\t MODE_LONG),\n-\t\tOPT_CMDMODE(0, \"name-only\", &cmdmode, N_(\"list only filenames\"),\n+\t\tOPT_CMDMODE(0, \"name-only\", &cmdmode_int, N_(\"list only filenames\"),\n \t\t\t MODE_NAME_ONLY),\n-\t\tOPT_CMDMODE(0, \"name-status\", &cmdmode, N_(\"list only filenames\"),\n+\t\tOPT_CMDMODE(0, \"name-status\", &cmdmode_int, N_(\"list only filenames\"),\n \t\t\t MODE_NAME_STATUS),\n-\t\tOPT_CMDMODE(0, \"object-only\", &cmdmode, N_(\"list only objects\"),\n+\t\tOPT_CMDMODE(0, \"object-only\", &cmdmode_int, N_(\"list only objects\"),\n \t\t\t MODE_OBJECT_ONLY),\n \t\tOPT_BOOL(0, \"full-name\", &full_name, N_(\"use full path names\")),\n \t\tOPT_BOOL(0, \"full-tree\", &full_tree,\n@@ -384,6 +385,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, ls_tree_options,\n \t\t\t ls_tree_usage, 0);\n \toptions.null_termination = null_termination;\n+\tcmdmode = cmdmode_int;\n\n \tif (full_tree)\n \t\tprefix = NULL;\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 50cb85751f..6dbe57f0ac 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1061,6 +1061,7 @@ static int check_exec_cmd(const char *cmd)\n int cmd_rebase(int argc, const char **argv, const char *prefix)\n {\n \tstruct rebase_options options = REBASE_OPTIONS_INIT;\n+\tint action;\n \tconst char *branch_name;\n \tint ret, flags, total_argc, in_progress = 0;\n \tint keep_base = 0;\n@@ -1116,18 +1117,18 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tOPT_BIT(0, \"no-ff\", &options.flags,\n \t\t\tN_(\"cherry-pick all commits, even if unchanged\"),\n \t\t\tREBASE_FORCE),\n-\t\tOPT_CMDMODE(0, \"continue\", &options.action, N_(\"continue\"),\n+\t\tOPT_CMDMODE(0, \"continue\", &action, N_(\"continue\"),\n \t\t\t ACTION_CONTINUE),\n-\t\tOPT_CMDMODE(0, \"skip\", &options.action,\n+\t\tOPT_CMDMODE(0, \"skip\", &action,\n \t\t\t N_(\"skip current patch and continue\"), ACTION_SKIP),\n-\t\tOPT_CMDMODE(0, \"abort\", &options.action,\n+\t\tOPT_CMDMODE(0, \"abort\", &action,\n \t\t\t N_(\"abort and check out the original branch\"),\n \t\t\t ACTION_ABORT),\n-\t\tOPT_CMDMODE(0, \"quit\", &options.action,\n+\t\tOPT_CMDMODE(0, \"quit\", &action,\n \t\t\t N_(\"abort but keep HEAD where it is\"), ACTION_QUIT),\n-\t\tOPT_CMDMODE(0, \"edit-todo\", &options.action, N_(\"edit the todo list \"\n+\t\tOPT_CMDMODE(0, \"edit-todo\", &action, N_(\"edit the todo list \"\n \t\t\t \"during an interactive rebase\"), ACTION_EDIT_TODO),\n-\t\tOPT_CMDMODE(0, \"show-current-patch\", &options.action,\n+\t\tOPT_CMDMODE(0, \"show-current-patch\", &action,\n \t\t\t N_(\"show the patch file being applied or merged\"),\n \t\t\t ACTION_SHOW_CURRENT_PATCH),\n \t\tOPT_CALLBACK_F(0, \"apply\", &options, NULL,\n@@ -1233,10 +1234,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tif (options.type != REBASE_UNSPECIFIED)\n \t\tin_progress = 1;\n\n+\taction = options.action;\n \ttotal_argc = argc;\n \targc = parse_options(argc, argv, prefix,\n \t\t\t builtin_rebase_options,\n \t\t\t builtin_rebase_usage, 0);\n+\toptions.action = action;\n\n \tif (preserve_merges_selected)\n \t\tdie(_(\"--preserve-merges was replaced by --rebase-merges\\n\"\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex da59600ad2..205c337ad3 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -554,12 +554,13 @@ int cmd_replace(int argc, const char **argv, const char *prefix)\n \t\tMODE_CONVERT_GRAFT_FILE,\n \t\tMODE_REPLACE\n \t} cmdmode = MODE_UNSPECIFIED;\n+\tint cmdmode_int = cmdmode;\n \tstruct option options[] = {\n-\t\tOPT_CMDMODE('l', \"list\", &cmdmode, N_(\"list replace refs\"), MODE_LIST),\n-\t\tOPT_CMDMODE('d', \"delete\", &cmdmode, N_(\"delete replace refs\"), MODE_DELETE),\n-\t\tOPT_CMDMODE('e', \"edit\", &cmdmode, N_(\"edit existing object\"), MODE_EDIT),\n-\t\tOPT_CMDMODE('g', \"graft\", &cmdmode, N_(\"change a commit's parents\"), MODE_GRAFT),\n-\t\tOPT_CMDMODE(0, \"convert-graft-file\", &cmdmode, N_(\"convert existing graft file\"), MODE_CONVERT_GRAFT_FILE),\n+\t\tOPT_CMDMODE('l', \"list\", &cmdmode_int, N_(\"list replace refs\"), MODE_LIST),\n+\t\tOPT_CMDMODE('d', \"delete\", &cmdmode_int, N_(\"delete replace refs\"), MODE_DELETE),\n+\t\tOPT_CMDMODE('e', \"edit\", &cmdmode_int, N_(\"edit existing object\"), MODE_EDIT),\n+\t\tOPT_CMDMODE('g', \"graft\", &cmdmode_int, N_(\"change a commit's parents\"), MODE_GRAFT),\n+\t\tOPT_CMDMODE(0, \"convert-graft-file\", &cmdmode_int, N_(\"convert existing graft file\"), MODE_CONVERT_GRAFT_FILE),\n \t\tOPT_BOOL_F('f', \"force\", &force, N_(\"replace the ref if it exists\"),\n \t\t\t PARSE_OPT_NOCOMPLETE),\n \t\tOPT_BOOL(0, \"raw\", &raw, N_(\"do not pretty-print contents for --edit\")),\n@@ -572,6 +573,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix)\n\n \targc = parse_options(argc, argv, prefix, options, git_replace_usage, 0);\n\n+\tcmdmode = cmdmode_int;\n \tif (!cmdmode)\n \t\tcmdmode = argc ? MODE_REPLACE : MODE_LIST;\n\ndiff --git a/builtin/stripspace.c b/builtin/stripspace.c\nindex 7b700a9fb1..e8efa0e7ac 100644\n--- a/builtin/stripspace.c\n+++ b/builtin/stripspace.c\n@@ -33,13 +33,14 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tenum stripspace_mode mode = STRIP_DEFAULT;\n+\tint mode_int = mode;\n \tint nongit;\n\n \tconst struct option options[] = {\n-\t\tOPT_CMDMODE('s', \"strip-comments\", &mode,\n+\t\tOPT_CMDMODE('s', \"strip-comments\", &mode_int,\n \t\t\t N_(\"skip and remove all lines starting with comment character\"),\n \t\t\t STRIP_COMMENTS),\n-\t\tOPT_CMDMODE('c', \"comment-lines\", &mode,\n+\t\tOPT_CMDMODE('c', \"comment-lines\", &mode_int,\n \t\t\t N_(\"prepend comment character and space to each line\"),\n \t\t\t COMMENT_LINES),\n \t\tOPT_END()\n@@ -49,6 +50,7 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)\n \tif (argc)\n \t\tusage_with_options(stripspace_usage, options);\n\n+\tmode = mode_int;\n \tif (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {\n \t\tsetup_git_directory_gently(&nongit);\n \t\tgit_config(git_default_config, NULL);\ndiff --git a/parse-options.h b/parse-options.h\nindex 5e7475bd2d..349c3fca04 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -262,7 +262,7 @@ struct option {\n \t.type = OPTION_SET_INT, \\\n \t.short_name = (s), \\\n \t.long_name = (l), \\\n-\t.value = (v), \\\n+\t.value_int = (v), \\\n \t.help = (h), \\\n \t.flags = PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG | (f), \\\n \t.defval = (i), \\\n--\n2.42.0\n"},{"id":"481945","messageId":"2349e897-9e0d-4341-86fc-9da117a1eb48@web.de","threadId":"60216","inReplyTo":"xmqq7cowv7pm.fsf@gitster.g","subject":"Re: [PATCH 1/2] parse-options: add int value pointer to struct option","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-09-18T09:53:19Z","receivedAt":"2023-09-18T09:54:53Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 11.09.23 um 21:19 schrieb Junio C Hamano:\n> Taylor Blau writes:\n>\n>> callback, something like:\n>>\n>> struct option {\n>> /* ... */\n>> union {\n>> void *value;\n>> int *value_int;\n>> /* etc ... */\n>> } u;\n>> enum option_type t;\n>> };\n>>\n>> where option_type has some value corresponding to \"void *\", another for\n>> \"int *\", and so on.\n>\n> Yup, that does cross my mind, even though I would have used\n>\n> \tunion {\n> \t\tvoid *void_ptr;\n> \t\tint *int_ptr;\n> \t} value;\n>\n> or something without a rather meaningless 'u'.\n\nOK, but I neglected to ask what we would get out of throwing different\ntypes into the same bin. It complicates type safety by making it\nimpossible for the parser to distinguish the used type. This will\nbecome relevant once all int options are converted and value_int can be\nmade mandatory for them. A named union also requires changing all\nusers.\n\nIt reduces the memory footprint, but only slightly. Saving a few bytes\nfor objects with less than a hundred instances total doesn't seem worth\nthe downsides.\n\nRené\n\n"},{"id":"481947","messageId":"ZQgiD0ivfYRpSbnJ@ugly","threadId":"60216","inReplyTo":"6dc558c6-f78c-4d9c-8444-498de8e4d22a@web.de","subject":"Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-09-18T10:10:23Z","receivedAt":"2023-09-18T10:11:27Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Mon, Sep 18, 2023 at 11:28:31AM +0200, René Scharfe wrote:\n>@@ -2300,12 +2301,12 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar\n> \t\t\t\t \"--show-current-patch\", arg);\n> \t}\n>\n>-\tif (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)\n>+\tif (resume->mode_int == RESUME_SHOW_PATCH && new_value != resume->sub_mode)\n>\nthis illustrates why i don't quite like the approach: the context \ndetermines which variable to use.\n\nmy idea would be to introduce a new type OPTION_SET_ENUM which would \nalso use the callback field. one could even adjust the data type and \nelide the callback when c23 mode (or more specifically, the enum size \nfeature) is detected.\n\n> \t\treturn error(_(\"options '%s=%s' and '%s=%s' \"\n> \t\t\t\t\t \"cannot be used together\"),\n\n> \t\t\t\t\t \"--show-current-patch\", \"--show-current-patch\", arg, valid_modes[resume->sub_mode]);\n>\ntotally on a tangent: the argument order is bogus here.\nand the line wrapping is also funny.\n\nregards\n"},{"id":"481949","messageId":"ZQgmQJvN0phJsFjz@ugly","threadId":"60216","inReplyTo":"2349e897-9e0d-4341-86fc-9da117a1eb48@web.de","subject":"Re: [PATCH 1/2] parse-options: add int value pointer to struct option","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-09-18T10:28:16Z","receivedAt":"2023-09-18T10:29:02Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Mon, Sep 18, 2023 at 11:53:19AM +0200, René Scharfe wrote:\n>Am 11.09.23 um 21:19 schrieb Junio C Hamano:\n>> Yup, that does cross my mind, even though I would have used\n>>\n>> \tunion {\n>> \t\tvoid *void_ptr;\n>> \t\tint *int_ptr;\n>> \t} value;\n>>\n>> or something without a rather meaningless 'u'.\n>\n>OK, but I neglected to ask what we would get out of throwing different\n>types into the same bin.\n\n>It complicates type safety by making it impossible for the parser to \n>distinguish the used type. [...]\n>\nthis is somewhat ironic, given that using a union has some semantic \nvalue by illustrating that the fields are mutually exclusive.\nbut i'm not sure that the checking is really important here, given that \nthe initializers are inside centralized macros anyway.\n\nregards\n"},{"id":"481950","messageId":"29780299-b3a2-4f4a-b236-ba6fbd24b6a3@app.fastmail.com","threadId":"60216","inReplyTo":"ZP+UgvIon1lrIFa+@ugly","subject":"Re: [PATCH 1/2] parse-options: add int value pointer to struct option","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-09-18T11:34:09Z","receivedAt":"2023-09-18T11:36:55Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Tue, Sep 12, 2023, at 00:28, Oswald Buddenhagen wrote:\n> i'd go the opposite way and make it an anonymous union.\n> that would require c11, though. imo not exactly an outrageous \n> proposition in 2023, but ...\n\nMoving to C11 would get pushback because not all platforms \nthat people care about support that compiler.[1]\n\n[1] https://lore.kernel.org/git/004601d8ed6b$13a2f580$3ae8e080$@nexbridge.com/ \n-- \nKristoffer Haugsbakk\n\n"},{"id":"481956","messageId":"0bf56c65-e59f-4290-8160-cce141f692d5@gmail.com","threadId":"60216","inReplyTo":"6dc558c6-f78c-4d9c-8444-498de8e4d22a@web.de","subject":"Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-09-18T13:33:56Z","receivedAt":"2023-09-18T16:07:11Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi René\n\nOn 18/09/2023 10:28, René Scharfe wrote:\n> Here's a version that preserves the enums by using additional int\n> variables just for the parsing phase. No tricks. The diff is long, but\n> most changes aren't particularly complicated and the resulting code is\n> not that ugly. Except for builtin/am.c perhaps, which changes the\n> command mode value using a callback as well.\n> \n\n> diff --git a/builtin/am.c b/builtin/am.c\n> index 202040b62e..00930e2152 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -2270,13 +2270,14 @@ enum resume_type {\n> \n> struct resume_mode {\n> \tenum resume_type mode;\n> +\tint mode_int;\n> \tenum show_patch_type sub_mode;\n> };\n> \n> static int parse_opt_show_current_patch(const struct option *opt, const char *arg, int unset)\n> {\n> \tint *opt_value = opt->value;\n> -\tstruct resume_mode *resume = container_of(opt_value, struct resume_mode, mode);\n> +\tstruct resume_mode *resume = container_of(opt_value, struct resume_mode, mode_int);\n> \n> \t/*\n> \t * Please update $__git_showcurrentpatch in git-completion.bash\n> @@ -2300,12 +2301,12 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar\n> \t\t\t\t \"--show-current-patch\", arg);\n> \t}\n> \n> -\tif (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)\n> +\tif (resume->mode_int == RESUME_SHOW_PATCH && new_value != resume->sub_mode)\n> \t\treturn error(_(\"options '%s=%s' and '%s=%s' \"\n> \t\t\t\t\t \"cannot be used together\"),\n> \t\t\t\t\t \"--show-current-patch\", \"--show-current-patch\", arg, valid_modes[resume->sub_mode]);\n> \n> -\tresume->mode = RESUME_SHOW_PATCH;\n> +\tresume->mode_int = RESUME_SHOW_PATCH;\n> \tresume->sub_mode = new_value;\n> \treturn 0;\n> }\n\nHaving \"mode\" and \"mode_int\" feels a bit fragile as only \"mode_int\" is \nvalid while parsing the options but then we want to use \"mode\". I wonder \nif we could get Oswald's idea of using callbacks working in a reasonably \nergonomic way with a couple of macros. We could add an new \nOPTION_SET_ENUM member to \"enum parse_opt_type\" that would take a setter \nfunction as well as the usual void *value. To set the value it would \npass the value pointer and an integer value to the setter function. We \ncould change OPT_CMDMODE to use OPTION_SET_ENUM and take the name of the \nenum as well as the integer value we want to set for that option. The \nname of the enum would be used to generate the name of the setter \ncallback which would be defined with another macro. The macro to \ngenerate the setter would look like\n\n#define MAKE_CMDMODE_SETTER(name) \\\n\tstatic void parse_cmdmode_ ## name (void * var, int value) {\n\t\tenum name *p = var;\n\t\t*p = value;\n\t}\n\nthen OPT_CMDMODE would look like\n\n#define OPT_CMDMODE_F(s, l, v, h, n, i, f) { \\\n\t.type = OPTION_SET_ENUM, \\\n\t.short_name = (s), \\\n\t.long_name = (l), \\\n\t.value = (v), \\\n\t.help = (h), \\\n\t.flags = PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG | (f), \\\n\t.defval = (i), \\\n\t.enum_setter = parse_cmdmode_ ## n,\n}\n#define OPT_CMDMODE(s, l, v, h, n, i) OPT_CMDMODE_F(s, l, v, h, n, i, 0)\n\n\nThen in builtin/am.c at the top level we'd add\n\nMAKE_CMDMODE_SETTER(resume_type)\n\nand change the option definitions to look like\n\nOPT_CMDMODE(0, \"continue\", resume_type, &resume.mode, ...)\n\n\nBest Wishes\n\nPhillip\n\n> @@ -2316,7 +2317,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n> \tint binary = -1;\n> \tint keep_cr = -1;\n> \tint patch_format = PATCH_FORMAT_UNKNOWN;\n> -\tstruct resume_mode resume = { .mode = RESUME_FALSE };\n> +\tstruct resume_mode resume = { .mode_int = RESUME_FALSE };\n> \tint in_progress;\n> \tint ret = 0;\n> \n> @@ -2387,27 +2388,27 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n> \t\t\tPARSE_OPT_NOARG),\n> \t\tOPT_STRING(0, \"resolvemsg\", &state.resolvemsg, NULL,\n> \t\t\tN_(\"override error message when patch failure occurs\")),\n> -\t\tOPT_CMDMODE(0, \"continue\", &resume.mode,\n> +\t\tOPT_CMDMODE(0, \"continue\", &resume.mode_int,\n> \t\t\tN_(\"continue applying patches after resolving a conflict\"),\n> \t\t\tRESUME_RESOLVED),\n> -\t\tOPT_CMDMODE('r', \"resolved\", &resume.mode,\n> +\t\tOPT_CMDMODE('r', \"resolved\", &resume.mode_int,\n> \t\t\tN_(\"synonyms for --continue\"),\n> \t\t\tRESUME_RESOLVED),\n> -\t\tOPT_CMDMODE(0, \"skip\", &resume.mode,\n> +\t\tOPT_CMDMODE(0, \"skip\", &resume.mode_int,\n> \t\t\tN_(\"skip the current patch\"),\n> \t\t\tRESUME_SKIP),\n> -\t\tOPT_CMDMODE(0, \"abort\", &resume.mode,\n> +\t\tOPT_CMDMODE(0, \"abort\", &resume.mode_int,\n> \t\t\tN_(\"restore the original branch and abort the patching operation\"),\n> \t\t\tRESUME_ABORT),\n> -\t\tOPT_CMDMODE(0, \"quit\", &resume.mode,\n> +\t\tOPT_CMDMODE(0, \"quit\", &resume.mode_int,\n> \t\t\tN_(\"abort the patching operation but keep HEAD where it is\"),\n> \t\t\tRESUME_QUIT),\n> -\t\t{ OPTION_CALLBACK, 0, \"show-current-patch\", &resume.mode,\n> +\t\t{ OPTION_CALLBACK, 0, \"show-current-patch\", &resume.mode_int,\n> \t\t \"(diff|raw)\",\n> \t\t N_(\"show the patch being applied\"),\n> \t\t PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,\n> \t\t parse_opt_show_current_patch, RESUME_SHOW_PATCH },\n> -\t\tOPT_CMDMODE(0, \"allow-empty\", &resume.mode,\n> +\t\tOPT_CMDMODE(0, \"allow-empty\", &resume.mode_int,\n> \t\t\tN_(\"record the empty patch as an empty commit\"),\n> \t\t\tRESUME_ALLOW_EMPTY),\n> \t\tOPT_BOOL(0, \"committer-date-is-author-date\",\n> @@ -2439,6 +2440,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n> \t\tam_load(&state);\n> \n> \targc = parse_options(argc, argv, prefix, options, usage, 0);\n> +\tresume.mode = resume.mode_int;\n> \n> \tif (binary >= 0)\n> \t\tfprintf_ln(stderr, _(\"The -b/--binary option has been a no-op for long time, and\\n\"\n> diff --git a/builtin/help.c b/builtin/help.c\n> index dc1fbe2b98..d76f88d544 100644\n> --- a/builtin/help.c\n> +++ b/builtin/help.c\n> @@ -51,6 +51,7 @@ static enum help_action {\n> \tHELP_ACTION_CONFIG_FOR_COMPLETION,\n> \tHELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION,\n> } cmd_mode;\n> +static int cmd_mode_int;\n> \n> static const char *html_path;\n> static int verbose = 1;\n> @@ -59,7 +60,7 @@ static int exclude_guides;\n> static int show_external_commands = -1;\n> static int show_aliases = -1;\n> static struct option builtin_help_options[] = {\n> -\tOPT_CMDMODE('a', \"all\", &cmd_mode, N_(\"print all available commands\"),\n> +\tOPT_CMDMODE('a', \"all\", &cmd_mode_int, N_(\"print all available commands\"),\n> \t\t HELP_ACTION_ALL),\n> \tOPT_BOOL(0, \"external-commands\", &show_external_commands,\n> \t\t N_(\"show external commands in --all\")),\n> @@ -72,19 +73,19 @@ static struct option builtin_help_options[] = {\n> \t\t\tHELP_FORMAT_INFO),\n> \tOPT__VERBOSE(&verbose, N_(\"print command description\")),\n> \n> -\tOPT_CMDMODE('g', \"guides\", &cmd_mode, N_(\"print list of useful guides\"),\n> +\tOPT_CMDMODE('g', \"guides\", &cmd_mode_int, N_(\"print list of useful guides\"),\n> \t\t HELP_ACTION_GUIDES),\n> -\tOPT_CMDMODE(0, \"user-interfaces\", &cmd_mode,\n> +\tOPT_CMDMODE(0, \"user-interfaces\", &cmd_mode_int,\n> \t\t N_(\"print list of user-facing repository, command and file interfaces\"),\n> \t\t HELP_ACTION_USER_INTERFACES),\n> -\tOPT_CMDMODE(0, \"developer-interfaces\", &cmd_mode,\n> +\tOPT_CMDMODE(0, \"developer-interfaces\", &cmd_mode_int,\n> \t\t N_(\"print list of file formats, protocols and other developer interfaces\"),\n> \t\t HELP_ACTION_DEVELOPER_INTERFACES),\n> -\tOPT_CMDMODE('c', \"config\", &cmd_mode, N_(\"print all configuration variable names\"),\n> +\tOPT_CMDMODE('c', \"config\", &cmd_mode_int, N_(\"print all configuration variable names\"),\n> \t\t HELP_ACTION_CONFIG),\n> -\tOPT_CMDMODE_F(0, \"config-for-completion\", &cmd_mode, \"\",\n> +\tOPT_CMDMODE_F(0, \"config-for-completion\", &cmd_mode_int, \"\",\n> \t\t HELP_ACTION_CONFIG_FOR_COMPLETION, PARSE_OPT_HIDDEN),\n> -\tOPT_CMDMODE_F(0, \"config-sections-for-completion\", &cmd_mode, \"\",\n> +\tOPT_CMDMODE_F(0, \"config-sections-for-completion\", &cmd_mode_int, \"\",\n> \t\t HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION, PARSE_OPT_HIDDEN),\n> \n> \tOPT_END(),\n> @@ -640,6 +641,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n> \targc = parse_options(argc, argv, prefix, builtin_help_options,\n> \t\t\tbuiltin_help_usage, 0);\n> \tparsed_help_format = help_format;\n> +\tcmd_mode = cmd_mode_int;\n> \n> \tif (cmd_mode != HELP_ACTION_ALL &&\n> \t (show_external_commands >= 0 ||\n> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c\n> index 209d2dc0d5..c64b38614a 100644\n> --- a/builtin/ls-tree.c\n> +++ b/builtin/ls-tree.c\n> @@ -346,7 +346,8 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n> \tint i, full_tree = 0;\n> \tint full_name = !prefix || !*prefix;\n> \tread_tree_fn_t fn = NULL;\n> -\tenum ls_tree_cmdmode cmdmode = MODE_DEFAULT;\n> +\tenum ls_tree_cmdmode cmdmode;\n> +\tint cmdmode_int = MODE_DEFAULT;\n> \tint null_termination = 0;\n> \tstruct ls_tree_options options = { 0 };\n> \tconst struct option ls_tree_options[] = {\n> @@ -358,13 +359,13 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n> \t\t\tLS_SHOW_TREES),\n> \t\tOPT_BOOL('z', NULL, &null_termination,\n> \t\t\t N_(\"terminate entries with NUL byte\")),\n> -\t\tOPT_CMDMODE('l', \"long\", &cmdmode, N_(\"include object size\"),\n> +\t\tOPT_CMDMODE('l', \"long\", &cmdmode_int, N_(\"include object size\"),\n> \t\t\t MODE_LONG),\n> -\t\tOPT_CMDMODE(0, \"name-only\", &cmdmode, N_(\"list only filenames\"),\n> +\t\tOPT_CMDMODE(0, \"name-only\", &cmdmode_int, N_(\"list only filenames\"),\n> \t\t\t MODE_NAME_ONLY),\n> -\t\tOPT_CMDMODE(0, \"name-status\", &cmdmode, N_(\"list only filenames\"),\n> +\t\tOPT_CMDMODE(0, \"name-status\", &cmdmode_int, N_(\"list only filenames\"),\n> \t\t\t MODE_NAME_STATUS),\n> -\t\tOPT_CMDMODE(0, \"object-only\", &cmdmode, N_(\"list only objects\"),\n> +\t\tOPT_CMDMODE(0, \"object-only\", &cmdmode_int, N_(\"list only objects\"),\n> \t\t\t MODE_OBJECT_ONLY),\n> \t\tOPT_BOOL(0, \"full-name\", &full_name, N_(\"use full path names\")),\n> \t\tOPT_BOOL(0, \"full-tree\", &full_tree,\n> @@ -384,6 +385,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n> \targc = parse_options(argc, argv, prefix, ls_tree_options,\n> \t\t\t ls_tree_usage, 0);\n> \toptions.null_termination = null_termination;\n> +\tcmdmode = cmdmode_int;\n> \n> \tif (full_tree)\n> \t\tprefix = NULL;\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 50cb85751f..6dbe57f0ac 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -1061,6 +1061,7 @@ static int check_exec_cmd(const char *cmd)\n> int cmd_rebase(int argc, const char **argv, const char *prefix)\n> {\n> \tstruct rebase_options options = REBASE_OPTIONS_INIT;\n> +\tint action;\n> \tconst char *branch_name;\n> \tint ret, flags, total_argc, in_progress = 0;\n> \tint keep_base = 0;\n> @@ -1116,18 +1117,18 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\tOPT_BIT(0, \"no-ff\", &options.flags,\n> \t\t\tN_(\"cherry-pick all commits, even if unchanged\"),\n> \t\t\tREBASE_FORCE),\n> -\t\tOPT_CMDMODE(0, \"continue\", &options.action, N_(\"continue\"),\n> +\t\tOPT_CMDMODE(0, \"continue\", &action, N_(\"continue\"),\n> \t\t\t ACTION_CONTINUE),\n> -\t\tOPT_CMDMODE(0, \"skip\", &options.action,\n> +\t\tOPT_CMDMODE(0, \"skip\", &action,\n> \t\t\t N_(\"skip current patch and continue\"), ACTION_SKIP),\n> -\t\tOPT_CMDMODE(0, \"abort\", &options.action,\n> +\t\tOPT_CMDMODE(0, \"abort\", &action,\n> \t\t\t N_(\"abort and check out the original branch\"),\n> \t\t\t ACTION_ABORT),\n> -\t\tOPT_CMDMODE(0, \"quit\", &options.action,\n> +\t\tOPT_CMDMODE(0, \"quit\", &action,\n> \t\t\t N_(\"abort but keep HEAD where it is\"), ACTION_QUIT),\n> -\t\tOPT_CMDMODE(0, \"edit-todo\", &options.action, N_(\"edit the todo list \"\n> +\t\tOPT_CMDMODE(0, \"edit-todo\", &action, N_(\"edit the todo list \"\n> \t\t\t \"during an interactive rebase\"), ACTION_EDIT_TODO),\n> -\t\tOPT_CMDMODE(0, \"show-current-patch\", &options.action,\n> +\t\tOPT_CMDMODE(0, \"show-current-patch\", &action,\n> \t\t\t N_(\"show the patch file being applied or merged\"),\n> \t\t\t ACTION_SHOW_CURRENT_PATCH),\n> \t\tOPT_CALLBACK_F(0, \"apply\", &options, NULL,\n> @@ -1233,10 +1234,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \tif (options.type != REBASE_UNSPECIFIED)\n> \t\tin_progress = 1;\n> \n> +\taction = options.action;\n> \ttotal_argc = argc;\n> \targc = parse_options(argc, argv, prefix,\n> \t\t\t builtin_rebase_options,\n> \t\t\t builtin_rebase_usage, 0);\n> +\toptions.action = action;\n> \n> \tif (preserve_merges_selected)\n> \t\tdie(_(\"--preserve-merges was replaced by --rebase-merges\\n\"\n> diff --git a/builtin/replace.c b/builtin/replace.c\n> index da59600ad2..205c337ad3 100644\n> --- a/builtin/replace.c\n> +++ b/builtin/replace.c\n> @@ -554,12 +554,13 @@ int cmd_replace(int argc, const char **argv, const char *prefix)\n> \t\tMODE_CONVERT_GRAFT_FILE,\n> \t\tMODE_REPLACE\n> \t} cmdmode = MODE_UNSPECIFIED;\n> +\tint cmdmode_int = cmdmode;\n> \tstruct option options[] = {\n> -\t\tOPT_CMDMODE('l', \"list\", &cmdmode, N_(\"list replace refs\"), MODE_LIST),\n> -\t\tOPT_CMDMODE('d', \"delete\", &cmdmode, N_(\"delete replace refs\"), MODE_DELETE),\n> -\t\tOPT_CMDMODE('e', \"edit\", &cmdmode, N_(\"edit existing object\"), MODE_EDIT),\n> -\t\tOPT_CMDMODE('g', \"graft\", &cmdmode, N_(\"change a commit's parents\"), MODE_GRAFT),\n> -\t\tOPT_CMDMODE(0, \"convert-graft-file\", &cmdmode, N_(\"convert existing graft file\"), MODE_CONVERT_GRAFT_FILE),\n> +\t\tOPT_CMDMODE('l', \"list\", &cmdmode_int, N_(\"list replace refs\"), MODE_LIST),\n> +\t\tOPT_CMDMODE('d', \"delete\", &cmdmode_int, N_(\"delete replace refs\"), MODE_DELETE),\n> +\t\tOPT_CMDMODE('e', \"edit\", &cmdmode_int, N_(\"edit existing object\"), MODE_EDIT),\n> +\t\tOPT_CMDMODE('g', \"graft\", &cmdmode_int, N_(\"change a commit's parents\"), MODE_GRAFT),\n> +\t\tOPT_CMDMODE(0, \"convert-graft-file\", &cmdmode_int, N_(\"convert existing graft file\"), MODE_CONVERT_GRAFT_FILE),\n> \t\tOPT_BOOL_F('f', \"force\", &force, N_(\"replace the ref if it exists\"),\n> \t\t\t PARSE_OPT_NOCOMPLETE),\n> \t\tOPT_BOOL(0, \"raw\", &raw, N_(\"do not pretty-print contents for --edit\")),\n> @@ -572,6 +573,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix)\n> \n> \targc = parse_options(argc, argv, prefix, options, git_replace_usage, 0);\n> \n> +\tcmdmode = cmdmode_int;\n> \tif (!cmdmode)\n> \t\tcmdmode = argc ? MODE_REPLACE : MODE_LIST;\n> \n> diff --git a/builtin/stripspace.c b/builtin/stripspace.c\n> index 7b700a9fb1..e8efa0e7ac 100644\n> --- a/builtin/stripspace.c\n> +++ b/builtin/stripspace.c\n> @@ -33,13 +33,14 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)\n> {\n> \tstruct strbuf buf = STRBUF_INIT;\n> \tenum stripspace_mode mode = STRIP_DEFAULT;\n> +\tint mode_int = mode;\n> \tint nongit;\n> \n> \tconst struct option options[] = {\n> -\t\tOPT_CMDMODE('s', \"strip-comments\", &mode,\n> +\t\tOPT_CMDMODE('s', \"strip-comments\", &mode_int,\n> \t\t\t N_(\"skip and remove all lines starting with comment character\"),\n> \t\t\t STRIP_COMMENTS),\n> -\t\tOPT_CMDMODE('c', \"comment-lines\", &mode,\n> +\t\tOPT_CMDMODE('c', \"comment-lines\", &mode_int,\n> \t\t\t N_(\"prepend comment character and space to each line\"),\n> \t\t\t COMMENT_LINES),\n> \t\tOPT_END()\n> @@ -49,6 +50,7 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)\n> \tif (argc)\n> \t\tusage_with_options(stripspace_usage, options);\n> \n> +\tmode = mode_int;\n> \tif (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {\n> \t\tsetup_git_directory_gently(&nongit);\n> \t\tgit_config(git_default_config, NULL);\n> diff --git a/parse-options.h b/parse-options.h\n> index 5e7475bd2d..349c3fca04 100644\n> --- a/parse-options.h\n> +++ b/parse-options.h\n> @@ -262,7 +262,7 @@ struct option {\n> \t.type = OPTION_SET_INT, \\\n> \t.short_name = (s), \\\n> \t.long_name = (l), \\\n> -\t.value = (v), \\\n> +\t.value_int = (v), \\\n> \t.help = (h), \\\n> \t.flags = PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG | (f), \\\n> \t.defval = (i), \\\n> --\n> 2.42.0\n\n"},{"id":"481961","messageId":"xmqq5y47mp58.fsf@gitster.g","threadId":"60216","inReplyTo":"2349e897-9e0d-4341-86fc-9da117a1eb48@web.de","subject":"Re: [PATCH 1/2] parse-options: add int value pointer to struct option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-18T16:17:55Z","receivedAt":"2023-09-18T16:36:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> It reduces the memory footprint, but only slightly. Saving a few bytes\n> for objects with less than a hundred instances total doesn't seem worth\n> the downsides.\n\nIt makes it impossible to use the both at the same time, which is a\nbigger (than reduced memory) advantage. Otherwise, we would be\ntempted to consider that having \"void *value\" and \"int value_int\"\nnext to each other and allow them to coexist may be a good solution\nfor a narrow corner case (please see at the end of the message you\nare responding to).\n\nAs you said, use of union has its downsides that may contradict the\nobjective of the larger picture this topic draws.\n\nThanks.\n"},{"id":"481965","messageId":"xmqqedivl832.fsf@gitster.g","threadId":"60216","inReplyTo":"0bf56c65-e59f-4290-8160-cce141f692d5@gmail.com","subject":"Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-18T17:11:45Z","receivedAt":"2023-09-18T17:11:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n>> -\tresume->mode = RESUME_SHOW_PATCH;\n>> +\tresume->mode_int = RESUME_SHOW_PATCH;\n>> \tresume->sub_mode = new_value;\n>> \treturn 0;\n>> }\n>\n> Having \"mode\" and \"mode_int\" feels a bit fragile as only \"mode_int\" is\n> valid while parsing the options but then we want to use \"mode\". I\n> wonder if we could get Oswald's idea of using callbacks working in a\n> reasonably ergonomic way with a couple of macros. We could add an new\n> OPTION_SET_ENUM member to \"enum parse_opt_type\" that would take a\n> setter function as well as the usual void *value. To set the value it\n> would pass the value pointer and an integer value to the setter\n> function. We could change OPT_CMDMODE to use OPTION_SET_ENUM and take\n> the name of the enum as well as the integer value we want to set for\n> that option. The name of the enum would be used to generate the name\n> of the setter callback which would be defined with another macro. The\n> macro to generate the setter would look like\n>\n> #define MAKE_CMDMODE_SETTER(name) \\\n> \tstatic void parse_cmdmode_ ## name (void * var, int value) {\n> \t\tenum name *p = var;\n> \t\t*p = value;\n> \t}\n\nAh, OK. So that's how you defeat \"how the size and alignment of an\nenum mixes well with int is not known and depends on particular enum\ntype\". It is a tad sad that this relies on \"void *\", which means\nthat the caller of parse_cmdmode_resume_type cannot be forced by the\ncompilers to pass \"enum resume_type *\" to the function, though. And\nthat is probably inevitable with the design as .enum_setter needs to\nbe of a single type, and the member in the \"struct option\" that\npoints at the destination variable must be \"void *\" as it has to\nbe capable of pointing at various different enum types.\n\n> ...\n> Then in builtin/am.c at the top level we'd add\n>\n> MAKE_CMDMODE_SETTER(resume_type)\n>\n> and change the option definitions to look like\n>\n> OPT_CMDMODE(0, \"continue\", resume_type, &resume.mode, ...)\n\nYup, that is ergonomic and corrects \"The shape of a particular enum\nmay not match 'int'\" issue nicely. I do not know how severe the\nproblem is that it is not quite type safe that we cannot enforce\nresume_type is the same as typeof(resume.mode) here, though.\n\nThanks.\n\n"},{"id":"481972","messageId":"000ff1b9-e7a5-4dd6-bc61-4b6761f66997@gmail.com","threadId":"60216","inReplyTo":"xmqqedivl832.fsf@gitster.g","subject":"Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-09-18T19:48:55Z","receivedAt":"2023-09-18T19:49:01Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 18/09/2023 18:11, Junio C Hamano wrote:\n>> Then in builtin/am.c at the top level we'd add\n>>\n>> MAKE_CMDMODE_SETTER(resume_type)\n>>\n>> and change the option definitions to look like\n>>\n>> OPT_CMDMODE(0, \"continue\", resume_type, &resume.mode, ...)\n> \n> Yup, that is ergonomic and corrects \"The shape of a particular enum\n> may not match 'int'\" issue nicely. I do not know how severe the\n> problem is that it is not quite type safe that we cannot enforce\n> resume_type is the same as typeof(resume.mode) here, though.\n\nWe could use gcc's __builtin_types_compatible_p() if we're prepared to \nhave two definitions of OPT_CMDMODE_F\n\n#if defined(__GNUC__)\n#define OPT_CMDMODE_F(s, l, n, v, h, i, f) { \\\n\t...\n\t.defval (i) + \\\n\t\tBUILD_ASSERT_OR_ZERO(__builtin_types_compatible_p(enum n, \n__typeof__(v))), \\\n}\n#else\n#define OPT_CMDMODE_F(s, l, n, v, h, i, f) { \\\n\t...\n\t.defval (i), \\\n}\n#endif\n\nBest Wishes\n\nPhillip\n"},{"id":"482005","messageId":"fff19abd-263d-48c7-81fd-35a2766b6b16@web.de","threadId":"60216","inReplyTo":"ZQgiD0ivfYRpSbnJ@ugly","subject":"Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-09-19T07:41:55Z","receivedAt":"2023-09-19T07:42:13Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 18.09.23 um 12:10 schrieb Oswald Buddenhagen:\n> On Mon, Sep 18, 2023 at 11:28:31AM +0200, René Scharfe wrote:\n>>\n>>         return error(_(\"options '%s=%s' and '%s=%s' \"\n>>                        \"cannot be used together\"),\n> \n>>                      \"--show-current-patch\", \"--show-current-patch\", arg, valid_modes[resume->sub_mode]);\n>>\n> totally on a tangent: the argument order is bogus here.\n> and the line wrapping is also funny.\n\nHah, good catch! Care to send a patch?\n\nRené\n"},{"id":"482007","messageId":"b948f3e5-7d03-4a2f-a719-963d52005291@web.de","threadId":"60216","inReplyTo":"0bf56c65-e59f-4290-8160-cce141f692d5@gmail.com","subject":"Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-09-19T07:47:00Z","receivedAt":"2023-09-19T07:47:16Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 18.09.23 um 15:33 schrieb Phillip Wood:\n> Hi René\n>\n> On 18/09/2023 10:28, René Scharfe wrote:\n>> Here's a version that preserves the enums by using additional int\n>> variables just for the parsing phase.  No tricks.  The diff is long, but\n>> most changes aren't particularly complicated and the resulting code is\n>> not that ugly.  Except for builtin/am.c perhaps, which changes the\n>> command mode value using a callback as well.\n>>\n>\n>> diff --git a/builtin/am.c b/builtin/am.c\n>> index 202040b62e..00930e2152 100644\n>> --- a/builtin/am.c\n>> +++ b/builtin/am.c\n>> @@ -2270,13 +2270,14 @@ enum resume_type {\n>>\n>>   struct resume_mode {\n>>       enum resume_type mode;\n>> +    int mode_int;\n>>       enum show_patch_type sub_mode;\n>>   };\n>>\n>>   static int parse_opt_show_current_patch(const struct option *opt, const char *arg, int unset)\n>>   {\n>>       int *opt_value = opt->value;\n>> -    struct resume_mode *resume = container_of(opt_value, struct resume_mode, mode);\n>> +    struct resume_mode *resume = container_of(opt_value, struct resume_mode, mode_int);\n>>\n>>       /*\n>>        * Please update $__git_showcurrentpatch in git-completion.bash\n>> @@ -2300,12 +2301,12 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar\n>>                        \"--show-current-patch\", arg);\n>>       }\n>>\n>> -    if (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)\n>> +    if (resume->mode_int == RESUME_SHOW_PATCH && new_value != resume->sub_mode)\n>>           return error(_(\"options '%s=%s' and '%s=%s' \"\n>>                          \"cannot be used together\"),\n>>                        \"--show-current-patch\", \"--show-current-patch\", arg, valid_modes[resume->sub_mode]);\n>>\n>> -    resume->mode = RESUME_SHOW_PATCH;\n>> +    resume->mode_int = RESUME_SHOW_PATCH;\n>>       resume->sub_mode = new_value;\n>>       return 0;\n>>   }\n>\n> Having \"mode\" and \"mode_int\" feels a bit fragile as only \"mode_int\"\n> is valid while parsing the options but then we want to use \"mode\".\n\nTrue. It feels a bit worse here than for the others because there the\nvariable is on the stack and here it's in a struct that is passed\naround.\n\n> I wonder if we could get Oswald's idea of using callbacks working in\n> a reasonably ergonomic way with a couple of macros. We could add an\n> new OPTION_SET_ENUM member to \"enum parse_opt_type\" that would take\n> a setter function as well as the usual void *value. To set the value\n> it would pass the value pointer and an integer value to the setter\n> function. We could change OPT_CMDMODE to use OPTION_SET_ENUM and\n> take the name of the enum as well as the integer value we want to set\n> for that option. The name of the enum would be used to generate the\n> name of the setter callback which would be defined with another\n> macro. The macro to generate the setter would look like\n>\n> #define MAKE_CMDMODE_SETTER(name) \\\n>     static void parse_cmdmode_ ## name (void * var, int value) {\n>         enum name *p = var;\n>         *p = value;\n>     }\n>\n> then OPT_CMDMODE would look like\n>\n> #define OPT_CMDMODE_F(s, l, v, h, n, i, f) { \\\n>     .type = OPTION_SET_ENUM, \\\n>     .short_name = (s), \\\n>     .long_name = (l), \\\n>     .value = (v), \\\n>     .help = (h), \\\n>     .flags = PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG | (f), \\\n>     .defval = (i), \\\n>     .enum_setter = parse_cmdmode_ ## n,\n> }\n> #define OPT_CMDMODE(s, l, v, h, n, i)  OPT_CMDMODE_F(s, l, v, h, n, i, 0)\n>\n>\n> Then in builtin/am.c at the top level we'd add\n>\n> MAKE_CMDMODE_SETTER(resume_type)\n>\n> and change the option definitions to look like\n>\n> OPT_CMDMODE(0, \"continue\", resume_type, &resume.mode, ...)\n\nAbout half of the OPT_CMDMODE users use int, not enum. They'd have to\nbe converted. On the flipside the direct and indirect uses of\nOPT_SET_INT_F with enums could be converted to an OPT_SET_ENUM_F based\non the above to avoid their use of incompatible pointers as well (e.g.\nOPT_IPVERSION).\n\nFor uses of OPT_BIT and OPT_NEGBIT with enums a getter would be needed\nas well, though (e.g. git ls-tree -d/-r/-t).\n\nAt this point I wonder if the existing callback mechanism would suffice.\nWhich brings me full circle to the topic of typed callbacks. Perhaps I\nshould introduce them first and come back to solving the int/enum issue\nat the end of that journey.\n\nRené\n"},{"id":"482010","messageId":"ZQlspgfu7yDW0oTN@ugly","threadId":"60216","inReplyTo":"e6d8a291-03de-cfd3-3813-747fc2cad145@web.de","subject":"Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-09-19T09:40:54Z","receivedAt":"2023-09-19T09:41:02Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Sat, Sep 09, 2023 at 11:14:20PM +0200, René Scharfe wrote:\n>Some uses of OPT_CMDMODE provide a pointer to an enum. It is\n>dereferenced as an int pointer in parse-options.c::get_value(). These\n>two types are incompatible, though\n>\ns/are/may be/ - c.f. https://en.cppreference.com/w/c/language/enum\n\n>-- the storage size of an enum can vary between platforms.\n>\nhere's a completely different perspective:\nthis is merely a theoretical problem, right? gcc for example won't \nactually use non-ints unless -fshort-enums is supplied. so how about \nsimply adding a (configure) test to ensure that there is actually no \nproblem, and calling it a day?\n\nregards\n\n"},{"id":"482050","messageId":"f778bc6f-dbe1-4df6-95ff-c9e9f36a3cc9@web.de","threadId":"60216","inReplyTo":"ZQlspgfu7yDW0oTN@ugly","subject":"Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-09-20T08:18:10Z","receivedAt":"2023-09-20T08:18:34Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 19.09.23 um 11:40 schrieb Oswald Buddenhagen:\n> On Sat, Sep 09, 2023 at 11:14:20PM +0200, René Scharfe wrote:\n>> Some uses of OPT_CMDMODE provide a pointer to an enum.  It is\n>> dereferenced as an int pointer in parse-options.c::get_value().  These\n>> two types are incompatible, though\n>>\n> s/are/may be/ - c.f. https://en.cppreference.com/w/c/language/enum\n\nYou're right. Citing the relevant part: \"Each enumerated type [...] is\ncompatible with one of: char, a signed integer type, or an unsigned\ninteger type [...]. It is implementation-defined which type is\ncompatible with any given enumerated type [...].\" So there's a chance\nthat the underlying type would be compatible by accident.\n\nWhen we try a few combinations (https://godbolt.org/z/KvKcndY4Y),\nClang warns about incompatible pointers if we use a pointer to an enum\nwith only positive values as int pointer and about different signs if\nuse a pointer to an enum with negative values as in unsigned int\npointer and accepts the rest. GCC accepts the same cases, but all its\nwarnings are about incompatible pointers. This seems to be dependent\non the optimization level, though. MSVC warns about all combinations.\n\n>> -- the storage size of an enum can vary between platforms.\n>>\n> here's a completely different perspective:\n> this is merely a theoretical problem, right? gcc for example won't\n> actually use non-ints unless -fshort-enums is supplied. so how about\n> simply adding a (configure) test to ensure that there is actually no\n> problem, and calling it a day?\n\nThat would be an easy, but complex solution. If the check is done\nusing -Wincompatible-pointer-types or equivalent then MSVC is out. If\nwe base it on type size then we're making assumptions that I find hard\nto justify. Using the same type at both ends of the void and avoiding\ncompiler warnings that would have been issued if we'd cut out the\nmiddle part is simpler overall.\n\nRené\n"},{"id":"482053","messageId":"daf41377-95ef-43ff-b4ce-a544a469a246@web.de","threadId":"60216","inReplyTo":"xmqq5y47mp58.fsf@gitster.g","subject":"Re: [PATCH 1/2] parse-options: add int value pointer to struct option","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-09-20T11:34:56Z","receivedAt":"2023-09-20T11:35:20Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 18.09.23 um 18:17 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>> It reduces the memory footprint, but only slightly. Saving a few bytes\n>> for objects with less than a hundred instances total doesn't seem worth\n>> the downsides.\n>\n> It makes it impossible to use the both at the same time, which is a\n> bigger (than reduced memory) advantage. Otherwise, we would be\n> tempted to consider that having \"void *value\" and \"int value_int\"\n> next to each other and allow them to coexist may be a good solution\n> for a narrow corner case (please see at the end of the message you\n> are responding to).\n\nGood point. The patch adds a check to parse_options_check() to prevent\ndouble use, which adds some runtime overhead and doesn't fully remove\nthe temptation.\n\nRené\n"},{"id":"482094","messageId":"20230921110727.789156-1-oswald.buddenhagen@gmx.de","threadId":"60216","inReplyTo":"fff19abd-263d-48c7-81fd-35a2766b6b16@web.de","subject":"[PATCH] am: fix error message in parse_opt_show_current_patch()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-09-21T11:07:27Z","receivedAt":"2023-09-21T17:50:04Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"The argument order was incorrect. This was introduced by 246cac8505\n(i18n: turn even more messages into \"cannot be used together\" ones,\n2022-01-05).\n\nSigned-off-by: Oswald Buddenhagen \n\n---\nfwiw, this is currently the only message that actually uses the %s=%s\nformat, so as of now, factoring out the argument names has only\ntheoretical value.\n\nCc: Jean-Noël Avila \nCc: Johannes Sixt \nCc: René Scharfe \nCc: Junio C Hamano \n---\n builtin/am.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 202040b62e..6655059a57 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -2303,7 +2303,8 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar\n \tif (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)\n \t\treturn error(_(\"options '%s=%s' and '%s=%s' \"\n \t\t\t\t\t \"cannot be used together\"),\n-\t\t\t\t\t \"--show-current-patch\", \"--show-current-patch\", arg, valid_modes[resume->sub_mode]);\n+\t\t\t \"--show-current-patch\", arg,\n+\t\t\t \"--show-current-patch\", valid_modes[resume->sub_mode]);\n \n \tresume->mode = RESUME_SHOW_PATCH;\n \tresume->sub_mode = new_value;\n-- \n2.42.0.419.g70bf8a5751\n\n"},{"id":"482095","messageId":"ZQwdsfh1GQX0IOQs@ugly","threadId":"60216","inReplyTo":"f778bc6f-dbe1-4df6-95ff-c9e9f36a3cc9@web.de","subject":"Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-09-21T10:40:49Z","receivedAt":"2023-09-21T17:51:18Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Wed, Sep 20, 2023 at 10:18:10AM +0200, René Scharfe wrote:\n> MSVC warns about all combinations.\n>\nyes, though that's not a problem: after we established that the \nunderlying type is int, we can just have a cast in the initializer \nmacro.\n\n>> so how about simply adding a (configure) test to ensure that there is \n>> actually no problem, and calling it a day?\n\n> If we base it on type size then we're making assumptions that I find \n> hard to justify.\n>\nthe only one i can think of is signedness. i think this can be safely \nignored as long as we use only small positive integers.\n\nregards\n"},{"id":"482100","messageId":"ZQyZcs/zXnPqc0Zd@ugly","threadId":"60216","inReplyTo":"xmqqh6nn5oo9.fsf@gitster.g","subject":"Re: [PATCH] am: fix error message in parse_opt_show_current_patch()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-09-21T19:28:50Z","receivedAt":"2023-09-21T19:42:45Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Thu, Sep 21, 2023 at 12:09:10PM -0700, Junio C Hamano wrote:\n>Oswald Buddenhagen writes:\n>> fwiw, this is currently the only message that actually uses the %s=%s\n>> format, so as of now, factoring out the argument names has only\n>> theoretical value.\n>\n>I am not sure I follow, if you mean that the programmer needs to\n>pass \"--show-current-patch\" only once if we used something like\n>\"%1$s=%2s and %1$s=%3s\", I agree that it probably has little value.\n>\nno, i mean that that the usual pattern is just \"options '%s' and '%s' \ncannot be used together\". this format string is indeed used many times, \nso it makes sense to factor out the option names to avoid duplication of \ntranslatable strings. not so here. but this particular case is still a \nlot less specialized than many of the other strings replaced by the \nreferenced patch, and it's at least plausible that further uses would be \nadded at some point, so i left it as-is.\n\ni thought about the duplication of the option string as well, but \ncompilers should merge the string constants, so that part is indeed \nde-duplicated. the cost of the extra pointer push could be avoided by \nuse of the %1$s syntax, but afaict that's unprecedented in git, and i \nkind of expect that some printf implementation would throw up from it.\nalso, it might reduce the chance of the format being used in another \nplace, but who knows.\n\nregards\n"},{"id":"482101","messageId":"xmqqh6nn5oo9.fsf@gitster.g","threadId":"60216","inReplyTo":"20230921110727.789156-1-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH] am: fix error message in parse_opt_show_current_patch()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-21T19:09:10Z","receivedAt":"2023-09-21T19:47:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oswald Buddenhagen writes:\n\n> The argument order was incorrect. This was introduced by 246cac8505\n> (i18n: turn even more messages into \"cannot be used together\" ones,\n> 2022-01-05).\n>\n> Signed-off-by: Oswald Buddenhagen \n>\n> ---\n> fwiw, this is currently the only message that actually uses the %s=%s\n> format, so as of now, factoring out the argument names has only\n> theoretical value.\n\nI am not sure I follow, if you mean that the programmer needs to\npass \"--show-current-patch\" only once if we used something like\n\"%1$s=%2s and %1$s=%3s\", I agree that it probably has little value.\n\nThe patch looks good. It seems that we are seeing a crack in test\ncoverage, by the way?\n\nWill queue. Thanks.\n\n> diff --git a/builtin/am.c b/builtin/am.c\n> index 202040b62e..6655059a57 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -2303,7 +2303,8 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar\n> \tif (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)\n> \t\treturn error(_(\"options '%s=%s' and '%s=%s' \"\n> \t\t\t\t\t \"cannot be used together\"),\n> -\t\t\t\t\t \"--show-current-patch\", \"--show-current-patch\", arg, valid_modes[resume->sub_mode]);\n> +\t\t\t \"--show-current-patch\", arg,\n> +\t\t\t \"--show-current-patch\", valid_modes[resume->sub_mode]);\n> \n> \tresume->mode = RESUME_SHOW_PATCH;\n> \tresume->sub_mode = new_value;\n"},{"id":"482563","messageId":"d9defed8-4e7e-4b84-be3d-57155d973320@web.de","threadId":"60216","inReplyTo":"ZQwdsfh1GQX0IOQs@ugly","subject":"Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-10-03T08:49:12Z","receivedAt":"2023-10-03T08:49:34Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 21.09.23 um 12:40 schrieb Oswald Buddenhagen:\n> On Wed, Sep 20, 2023 at 10:18:10AM +0200, René Scharfe wrote:\n>> MSVC warns about all combinations.\n>>\n> yes, though that's not a problem: after we established that the\n> underlying type is int, we can just have a cast in the initializer\n> macro.\n\nMSVC does some weird things in general; it's tempting to ignore it.\n\n>>> so how about simply adding a (configure) test to ensure that\n>>> there is actually no problem, and calling it a day?\n>\n>> If we base it on type size then we're making assumptions that I\n>> find hard to justify.\n>>\n> the only one i can think of is signedness. i think this can be safely\n> ignored as long as we use only small positive integers.\n\nI don't fully understand the pointer-sign warning, so I'm not\nconfident enough to silence it.\n\nRené\n"},{"id":"482564","messageId":"6cb09270-04b9-456e-8d7e-97137e56e9e2@web.de","threadId":"60216","inReplyTo":"000ff1b9-e7a5-4dd6-bc61-4b6761f66997@gmail.com","subject":"Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-10-03T08:49:15Z","receivedAt":"2023-10-03T08:49:40Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 18.09.23 um 21:48 schrieb Phillip Wood:\n> On 18/09/2023 18:11, Junio C Hamano wrote:\n>>> Then in builtin/am.c at the top level we'd add\n>>>\n>>> MAKE_CMDMODE_SETTER(resume_type)\n>>>\n>>> and change the option definitions to look like\n>>>\n>>> OPT_CMDMODE(0, \"continue\", resume_type, &resume.mode, ...)\n>>\n>> Yup, that is ergonomic and corrects \"The shape of a particular enum\n>> may not match 'int'\" issue nicely.  I do not know how severe the\n>> problem is that it is not quite type safe that we cannot enforce\n>> resume_type is the same as typeof(resume.mode) here, though.\n>\n> We could use gcc's __builtin_types_compatible_p() if we're prepared to have two definitions of OPT_CMDMODE_F\n>\n> #if defined(__GNUC__)\n> #define OPT_CMDMODE_F(s, l, n, v, h, i, f) { \\\n>     ...\n>     .defval (i) + \\\n>         BUILD_ASSERT_OR_ZERO(__builtin_types_compatible_p(enum n, __typeof__(v))), \\\n> }\n> #else\n> #define OPT_CMDMODE_F(s, l, n, v, h, i, f) { \\\n>     ...\n>     .defval (i), \\\n> }\n> #endif\n>\n> Best Wishes\n>\n> Phillip\n\nThat would work, but we can do a type check without a compiler builtin\nby creating a function for that purpose. How about this?\n\n--- >8 ---\nSubject: [PATCH] parse-options: make OPT_CMDMODE type-safe\n\nSome uses of OPT_CMDMODE point to an enum as value, but the option\nparser dereferences it as an int pointer. Depending on the platform,\npointers to int and enums can be incompatible.\n\nAdd value accessor functions to struct option to dereference the pointer\nsafely, use them for PARSE_OPT_CMDMODE error detection and in the new\nOPTION_SET_VALUE option type, add a typed version of OPT_CMDMODE named\nOPT_CMDMODE_T, make OPT_CMDMODE only accept int and convert enum users\nto declare the appropriate type.\n\nThe accessor functions for each type must be defined using\nDEFINE_OPTION_VALUE_TYPE. OPT_CMDMODE needs the ones for int, so we\ndefine them centrally in parse-options.h.\n\nbuiltin/am.c::parse_opt_show_current_patch() uses the value pointer just\nto calculate the offset of the enclosing struct resume_mode, making the\npointer type inconsequential. Use the correct type instead of int *\nregardless, for consistency.\n\nThe enum used with OPT_CMDMODE in builtin/replace.c was anonymous. Give\nit a name to allow specifying it in DEFINE_OPTION_VALUE_TYPE and then\nOPT_CMDMODE_T.\n\nSuggested-by: Oswald Buddenhagen \nSigned-off-by: René Scharfe \n---\n Documentation/technical/api-parse-options.txt | 7 ++++\n builtin/am.c | 32 ++++++++++------\n builtin/help.c | 37 +++++++++++--------\n builtin/ls-tree.c | 18 +++++----\n builtin/rebase.c | 33 ++++++++++-------\n builtin/replace.c | 37 ++++++++++++-------\n builtin/stripspace.c | 14 ++++---\n parse-options.c | 20 +++++++++-\n parse-options.h | 36 ++++++++++++++++--\n 9 files changed, 159 insertions(+), 75 deletions(-)\n\ndiff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt\nindex 61fa6ee167..3958ab7c94 100644\n--- a/Documentation/technical/api-parse-options.txt\n+++ b/Documentation/technical/api-parse-options.txt\n@@ -278,6 +278,13 @@ There are some macros to easily define options:\n \toption has already set its value to the same `int_var`.\n \tIn new commands consider using subcommands instead.\n\n+`OPT_CMDMODE_T(short, long, type_name, &enum_var, description, enum_val)`::\n+\tSame as `OPT_CMDMODE`, but allows specifying an enum variable\n+\tinstead of an int.\n+\tRequires `DEFINE_OPTION_VALUE_TYPE(type_name, enum_type);` at\n+\tfile scope to declare `type_name`; `enum_type` must be the type\n+\tof `enum_var`.\n+\n `OPT_SUBCOMMAND(long, &fn_ptr, subcommand_fn)`::\n \tDefine a subcommand. `subcommand_fn` is put into `fn_ptr` when\n \tthis subcommand is used.\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 6655059a57..4642dc5099 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -2268,6 +2268,8 @@ enum resume_type {\n \tRESUME_ALLOW_EMPTY,\n };\n\n+DEFINE_OPTION_VALUE_TYPE(resume_type, enum resume_type);\n+\n struct resume_mode {\n \tenum resume_type mode;\n \tenum show_patch_type sub_mode;\n@@ -2275,7 +2277,7 @@ struct resume_mode {\n\n static int parse_opt_show_current_patch(const struct option *opt, const char *arg, int unset)\n {\n-\tint *opt_value = opt->value;\n+\tenum resume_type *opt_value = opt->value;\n \tstruct resume_mode *resume = container_of(opt_value, struct resume_mode, mode);\n\n \t/*\n@@ -2388,27 +2390,33 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\t\tPARSE_OPT_NOARG),\n \t\tOPT_STRING(0, \"resolvemsg\", &state.resolvemsg, NULL,\n \t\t\tN_(\"override error message when patch failure occurs\")),\n-\t\tOPT_CMDMODE(0, \"continue\", &resume.mode,\n+\t\tOPT_CMDMODE_T(0, \"continue\", resume_type, &resume.mode,\n \t\t\tN_(\"continue applying patches after resolving a conflict\"),\n \t\t\tRESUME_RESOLVED),\n-\t\tOPT_CMDMODE('r', \"resolved\", &resume.mode,\n+\t\tOPT_CMDMODE_T('r', \"resolved\", resume_type, &resume.mode,\n \t\t\tN_(\"synonyms for --continue\"),\n \t\t\tRESUME_RESOLVED),\n-\t\tOPT_CMDMODE(0, \"skip\", &resume.mode,\n+\t\tOPT_CMDMODE_T(0, \"skip\", resume_type, &resume.mode,\n \t\t\tN_(\"skip the current patch\"),\n \t\t\tRESUME_SKIP),\n-\t\tOPT_CMDMODE(0, \"abort\", &resume.mode,\n+\t\tOPT_CMDMODE_T(0, \"abort\", resume_type, &resume.mode,\n \t\t\tN_(\"restore the original branch and abort the patching operation\"),\n \t\t\tRESUME_ABORT),\n-\t\tOPT_CMDMODE(0, \"quit\", &resume.mode,\n+\t\tOPT_CMDMODE_T(0, \"quit\", resume_type, &resume.mode,\n \t\t\tN_(\"abort the patching operation but keep HEAD where it is\"),\n \t\t\tRESUME_QUIT),\n-\t\t{ OPTION_CALLBACK, 0, \"show-current-patch\", &resume.mode,\n-\t\t \"(diff|raw)\",\n-\t\t N_(\"show the patch being applied\"),\n-\t\t PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,\n-\t\t parse_opt_show_current_patch, RESUME_SHOW_PATCH },\n-\t\tOPT_CMDMODE(0, \"allow-empty\", &resume.mode,\n+\t\t{\n+\t\t\t.type = OPTION_CALLBACK,\n+\t\t\t.long_name = \"show-current-patch\",\n+\t\t\tOPTION_VALUE(resume_type, &resume.mode),\n+\t\t\t.argh = \"(diff|raw)\",\n+\t\t\t.help = N_(\"show the patch being applied\"),\n+\t\t\t.flags = PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG |\n+\t\t\t\t PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,\n+\t\t\t.callback = parse_opt_show_current_patch,\n+\t\t\t.defval = RESUME_SHOW_PATCH,\n+\t\t},\n+\t\tOPT_CMDMODE_T(0, \"allow-empty\", resume_type, &resume.mode,\n \t\t\tN_(\"record the empty patch as an empty commit\"),\n \t\t\tRESUME_ALLOW_EMPTY),\n \t\tOPT_BOOL(0, \"committer-date-is-author-date\",\ndiff --git a/builtin/help.c b/builtin/help.c\nindex dc1fbe2b98..48437c8636 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -52,6 +52,8 @@ static enum help_action {\n \tHELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION,\n } cmd_mode;\n\n+DEFINE_OPTION_VALUE_TYPE(help_action, enum help_action);\n+\n static const char *html_path;\n static int verbose = 1;\n static enum help_format help_format = HELP_FORMAT_NONE;\n@@ -59,8 +61,8 @@ static int exclude_guides;\n static int show_external_commands = -1;\n static int show_aliases = -1;\n static struct option builtin_help_options[] = {\n-\tOPT_CMDMODE('a', \"all\", &cmd_mode, N_(\"print all available commands\"),\n-\t\t HELP_ACTION_ALL),\n+\tOPT_CMDMODE_T('a', \"all\", help_action, &cmd_mode,\n+\t\t N_(\"print all available commands\"), HELP_ACTION_ALL),\n \tOPT_BOOL(0, \"external-commands\", &show_external_commands,\n \t\t N_(\"show external commands in --all\")),\n \tOPT_BOOL(0, \"aliases\", &show_aliases, N_(\"show aliases in --all\")),\n@@ -72,20 +74,23 @@ static struct option builtin_help_options[] = {\n \t\t\tHELP_FORMAT_INFO),\n \tOPT__VERBOSE(&verbose, N_(\"print command description\")),\n\n-\tOPT_CMDMODE('g', \"guides\", &cmd_mode, N_(\"print list of useful guides\"),\n-\t\t HELP_ACTION_GUIDES),\n-\tOPT_CMDMODE(0, \"user-interfaces\", &cmd_mode,\n-\t\t N_(\"print list of user-facing repository, command and file interfaces\"),\n-\t\t HELP_ACTION_USER_INTERFACES),\n-\tOPT_CMDMODE(0, \"developer-interfaces\", &cmd_mode,\n-\t\t N_(\"print list of file formats, protocols and other developer interfaces\"),\n-\t\t HELP_ACTION_DEVELOPER_INTERFACES),\n-\tOPT_CMDMODE('c', \"config\", &cmd_mode, N_(\"print all configuration variable names\"),\n-\t\t HELP_ACTION_CONFIG),\n-\tOPT_CMDMODE_F(0, \"config-for-completion\", &cmd_mode, \"\",\n-\t\t HELP_ACTION_CONFIG_FOR_COMPLETION, PARSE_OPT_HIDDEN),\n-\tOPT_CMDMODE_F(0, \"config-sections-for-completion\", &cmd_mode, \"\",\n-\t\t HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION, PARSE_OPT_HIDDEN),\n+\tOPT_CMDMODE_T('g', \"guides\", help_action, &cmd_mode,\n+\t\t N_(\"print list of useful guides\"), HELP_ACTION_GUIDES),\n+\tOPT_CMDMODE_T(0, \"user-interfaces\", help_action, &cmd_mode,\n+\t\t N_(\"print list of user-facing repository, command and file interfaces\"),\n+\t\t HELP_ACTION_USER_INTERFACES),\n+\tOPT_CMDMODE_T(0, \"developer-interfaces\", help_action, &cmd_mode,\n+\t\t N_(\"print list of file formats, protocols and other developer interfaces\"),\n+\t\t HELP_ACTION_DEVELOPER_INTERFACES),\n+\tOPT_CMDMODE_T('c', \"config\", help_action, &cmd_mode,\n+\t\t N_(\"print all configuration variable names\"),\n+\t\t HELP_ACTION_CONFIG),\n+\tOPT_CMDMODE_T_F(0, \"config-for-completion\", help_action, &cmd_mode, \"\",\n+\t\t\tHELP_ACTION_CONFIG_FOR_COMPLETION, PARSE_OPT_HIDDEN),\n+\tOPT_CMDMODE_T_F(0, \"config-sections-for-completion\",\n+\t\t\thelp_action, &cmd_mode, \"\",\n+\t\t\tHELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION,\n+\t\t\tPARSE_OPT_HIDDEN),\n\n \tOPT_END(),\n };\ndiff --git a/builtin/ls-tree.c b/builtin/ls-tree.c\nindex 209d2dc0d5..2e7faf510d 100644\n--- a/builtin/ls-tree.c\n+++ b/builtin/ls-tree.c\n@@ -339,6 +339,8 @@ static struct ls_tree_cmdmode_to_fmt ls_tree_cmdmode_format[] = {\n \t},\n };\n\n+DEFINE_OPTION_VALUE_TYPE(cmdmode, enum ls_tree_cmdmode);\n+\n int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n {\n \tstruct object_id oid;\n@@ -358,14 +360,14 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n \t\t\tLS_SHOW_TREES),\n \t\tOPT_BOOL('z', NULL, &null_termination,\n \t\t\t N_(\"terminate entries with NUL byte\")),\n-\t\tOPT_CMDMODE('l', \"long\", &cmdmode, N_(\"include object size\"),\n-\t\t\t MODE_LONG),\n-\t\tOPT_CMDMODE(0, \"name-only\", &cmdmode, N_(\"list only filenames\"),\n-\t\t\t MODE_NAME_ONLY),\n-\t\tOPT_CMDMODE(0, \"name-status\", &cmdmode, N_(\"list only filenames\"),\n-\t\t\t MODE_NAME_STATUS),\n-\t\tOPT_CMDMODE(0, \"object-only\", &cmdmode, N_(\"list only objects\"),\n-\t\t\t MODE_OBJECT_ONLY),\n+\t\tOPT_CMDMODE_T('l', \"long\", cmdmode, &cmdmode,\n+\t\t\t N_(\"include object size\"), MODE_LONG),\n+\t\tOPT_CMDMODE_T(0, \"name-only\", cmdmode, &cmdmode,\n+\t\t\t N_(\"list only filenames\"), MODE_NAME_ONLY),\n+\t\tOPT_CMDMODE_T(0, \"name-status\", cmdmode, &cmdmode,\n+\t\t\t N_(\"list only filenames\"), MODE_NAME_STATUS),\n+\t\tOPT_CMDMODE_T(0, \"object-only\", cmdmode, &cmdmode,\n+\t\t\t N_(\"list only objects\"), MODE_OBJECT_ONLY),\n \t\tOPT_BOOL(0, \"full-name\", &full_name, N_(\"use full path names\")),\n \t\tOPT_BOOL(0, \"full-tree\", &full_tree,\n \t\t\t N_(\"list entire tree; not just current directory \"\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex ed15accec9..50739327bd 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -75,6 +75,8 @@ enum action {\n \tACTION_SHOW_CURRENT_PATCH\n };\n\n+DEFINE_OPTION_VALUE_TYPE(action, enum action);\n+\n static const char *action_names[] = {\n \t\"undefined\",\n \t\"continue\",\n@@ -1116,20 +1118,23 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tOPT_BIT(0, \"no-ff\", &options.flags,\n \t\t\tN_(\"cherry-pick all commits, even if unchanged\"),\n \t\t\tREBASE_FORCE),\n-\t\tOPT_CMDMODE(0, \"continue\", &options.action, N_(\"continue\"),\n-\t\t\t ACTION_CONTINUE),\n-\t\tOPT_CMDMODE(0, \"skip\", &options.action,\n-\t\t\t N_(\"skip current patch and continue\"), ACTION_SKIP),\n-\t\tOPT_CMDMODE(0, \"abort\", &options.action,\n-\t\t\t N_(\"abort and check out the original branch\"),\n-\t\t\t ACTION_ABORT),\n-\t\tOPT_CMDMODE(0, \"quit\", &options.action,\n-\t\t\t N_(\"abort but keep HEAD where it is\"), ACTION_QUIT),\n-\t\tOPT_CMDMODE(0, \"edit-todo\", &options.action, N_(\"edit the todo list \"\n-\t\t\t \"during an interactive rebase\"), ACTION_EDIT_TODO),\n-\t\tOPT_CMDMODE(0, \"show-current-patch\", &options.action,\n-\t\t\t N_(\"show the patch file being applied or merged\"),\n-\t\t\t ACTION_SHOW_CURRENT_PATCH),\n+\t\tOPT_CMDMODE_T(0, \"continue\", action, &options.action,\n+\t\t\t N_(\"continue\"), ACTION_CONTINUE),\n+\t\tOPT_CMDMODE_T(0, \"skip\", action, &options.action,\n+\t\t\t N_(\"skip current patch and continue\"),\n+\t\t\t ACTION_SKIP),\n+\t\tOPT_CMDMODE_T(0, \"abort\", action, &options.action,\n+\t\t\t N_(\"abort and check out the original branch\"),\n+\t\t\t ACTION_ABORT),\n+\t\tOPT_CMDMODE_T(0, \"quit\", action, &options.action,\n+\t\t\t N_(\"abort but keep HEAD where it is\"),\n+\t\t\t ACTION_QUIT),\n+\t\tOPT_CMDMODE_T(0, \"edit-todo\", action, &options.action,\n+\t\t\t N_(\"edit the todo list during an interactive rebase\"),\n+\t\t\t ACTION_EDIT_TODO),\n+\t\tOPT_CMDMODE_T(0, \"show-current-patch\", action, &options.action,\n+\t\t\t N_(\"show the patch file being applied or merged\"),\n+\t\t\t ACTION_SHOW_CURRENT_PATCH),\n \t\tOPT_CALLBACK_F(0, \"apply\", &options, NULL,\n \t\t\tN_(\"use apply strategies to rebase\"),\n \t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG,\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex da59600ad2..09aebc9de2 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -539,27 +539,36 @@ static int convert_graft_file(int force)\n\n \treturn -1;\n }\n+enum cmdmode {\n+\tMODE_UNSPECIFIED = 0,\n+\tMODE_LIST,\n+\tMODE_DELETE,\n+\tMODE_EDIT,\n+\tMODE_GRAFT,\n+\tMODE_CONVERT_GRAFT_FILE,\n+\tMODE_REPLACE\n+};\n+\n+DEFINE_OPTION_VALUE_TYPE(cmdmode, enum cmdmode);\n\n int cmd_replace(int argc, const char **argv, const char *prefix)\n {\n \tint force = 0;\n \tint raw = 0;\n \tconst char *format = NULL;\n-\tenum {\n-\t\tMODE_UNSPECIFIED = 0,\n-\t\tMODE_LIST,\n-\t\tMODE_DELETE,\n-\t\tMODE_EDIT,\n-\t\tMODE_GRAFT,\n-\t\tMODE_CONVERT_GRAFT_FILE,\n-\t\tMODE_REPLACE\n-\t} cmdmode = MODE_UNSPECIFIED;\n+\tenum cmdmode cmdmode = MODE_UNSPECIFIED;\n \tstruct option options[] = {\n-\t\tOPT_CMDMODE('l', \"list\", &cmdmode, N_(\"list replace refs\"), MODE_LIST),\n-\t\tOPT_CMDMODE('d', \"delete\", &cmdmode, N_(\"delete replace refs\"), MODE_DELETE),\n-\t\tOPT_CMDMODE('e', \"edit\", &cmdmode, N_(\"edit existing object\"), MODE_EDIT),\n-\t\tOPT_CMDMODE('g', \"graft\", &cmdmode, N_(\"change a commit's parents\"), MODE_GRAFT),\n-\t\tOPT_CMDMODE(0, \"convert-graft-file\", &cmdmode, N_(\"convert existing graft file\"), MODE_CONVERT_GRAFT_FILE),\n+\t\tOPT_CMDMODE_T('l', \"list\", cmdmode, &cmdmode,\n+\t\t\t N_(\"list replace refs\"), MODE_LIST),\n+\t\tOPT_CMDMODE_T('d', \"delete\", cmdmode, &cmdmode,\n+\t\t\t N_(\"delete replace refs\"), MODE_DELETE),\n+\t\tOPT_CMDMODE_T('e', \"edit\", cmdmode, &cmdmode,\n+\t\t\t N_(\"edit existing object\"), MODE_EDIT),\n+\t\tOPT_CMDMODE_T('g', \"graft\", cmdmode, &cmdmode,\n+\t\t\t N_(\"change a commit's parents\"), MODE_GRAFT),\n+\t\tOPT_CMDMODE_T(0, \"convert-graft-file\", cmdmode, &cmdmode,\n+\t\t\t N_(\"convert existing graft file\"),\n+\t\t\t MODE_CONVERT_GRAFT_FILE),\n \t\tOPT_BOOL_F('f', \"force\", &force, N_(\"replace the ref if it exists\"),\n \t\t\t PARSE_OPT_NOCOMPLETE),\n \t\tOPT_BOOL(0, \"raw\", &raw, N_(\"do not pretty-print contents for --edit\")),\ndiff --git a/builtin/stripspace.c b/builtin/stripspace.c\nindex 7b700a9fb1..82b9a95a4f 100644\n--- a/builtin/stripspace.c\n+++ b/builtin/stripspace.c\n@@ -29,6 +29,8 @@ enum stripspace_mode {\n \tCOMMENT_LINES\n };\n\n+DEFINE_OPTION_VALUE_TYPE(mode, enum stripspace_mode);\n+\n int cmd_stripspace(int argc, const char **argv, const char *prefix)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -36,12 +38,12 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)\n \tint nongit;\n\n \tconst struct option options[] = {\n-\t\tOPT_CMDMODE('s', \"strip-comments\", &mode,\n-\t\t\t N_(\"skip and remove all lines starting with comment character\"),\n-\t\t\t STRIP_COMMENTS),\n-\t\tOPT_CMDMODE('c', \"comment-lines\", &mode,\n-\t\t\t N_(\"prepend comment character and space to each line\"),\n-\t\t\t COMMENT_LINES),\n+\t\tOPT_CMDMODE_T('s', \"strip-comments\", mode, &mode,\n+\t\t\t N_(\"skip and remove all lines starting with comment character\"),\n+\t\t\t STRIP_COMMENTS),\n+\t\tOPT_CMDMODE_T('c', \"comment-lines\", mode, &mode,\n+\t\t\t N_(\"prepend comment character and space to each line\"),\n+\t\t\t COMMENT_LINES),\n \t\tOPT_END()\n \t};\n\ndiff --git a/parse-options.c b/parse-options.c\nindex e8e076c3a6..63a2247128 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -85,7 +85,7 @@ static enum parse_opt_result opt_command_mode_error(\n \t\tif (that == opt ||\n \t\t !(that->flags & PARSE_OPT_CMDMODE) ||\n \t\t that->value != opt->value ||\n-\t\t that->defval != *(int *)opt->value)\n+\t\t that->defval != opt->get_value(opt->value))\n \t\t\tcontinue;\n\n \t\tif (that->long_name)\n@@ -122,7 +122,8 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n \t * is not a grave error, so let it pass.\n \t */\n \tif ((opt->flags & PARSE_OPT_CMDMODE) &&\n-\t *(int *)opt->value && *(int *)opt->value != opt->defval)\n+\t opt->get_value(opt->value) &&\n+\t opt->get_value(opt->value) != opt->defval)\n \t\treturn opt_command_mode_error(opt, all_opts, flags);\n\n \tswitch (opt->type) {\n@@ -160,6 +161,10 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n \t\t*(int *)opt->value = unset ? 0 : opt->defval;\n \t\treturn 0;\n\n+\tcase OPTION_SET_VALUE:\n+\t\topt->set_value(opt->value, unset ? 0 : opt->defval);\n+\t\treturn 0;\n+\n \tcase OPTION_STRING:\n \t\tif (unset)\n \t\t\t*(const char **)opt->value = NULL;\n@@ -483,11 +488,21 @@ static void parse_options_check(const struct option *opts)\n \t\tif (opts->type == OPTION_SET_INT && !opts->defval &&\n \t\t opts->long_name && !(opts->flags & PARSE_OPT_NONEG))\n \t\t\toptbug(opts, \"OPTION_SET_INT 0 should not be negatable\");\n+\t\tif (opts->type == OPTION_SET_VALUE && !opts->defval &&\n+\t\t opts->long_name && !(opts->flags & PARSE_OPT_NONEG))\n+\t\t\toptbug(opts, \"OPTION_SET_VALUE 0 should not be negatable\");\n+\t\tif (opts->type == OPTION_SET_VALUE &&\n+\t\t (!opts->get_value || !opts->set_value))\n+\t\t\toptbug(opts, \"OPTION_SET_VALUE requires accessors\");\n+\t\tif ((opts->flags & PARSE_OPT_CMDMODE) &&\n+\t\t (!opts->get_value || !opts->set_value))\n+\t\t\toptbug(opts, \"PARSE_OPT_CMDMODE requires accessors\");\n \t\tswitch (opts->type) {\n \t\tcase OPTION_COUNTUP:\n \t\tcase OPTION_BIT:\n \t\tcase OPTION_NEGBIT:\n \t\tcase OPTION_SET_INT:\n+\t\tcase OPTION_SET_VALUE:\n \t\tcase OPTION_NUMBER:\n \t\t\tif ((opts->flags & PARSE_OPT_OPTARG) ||\n \t\t\t !(opts->flags & PARSE_OPT_NOARG))\n@@ -611,6 +626,7 @@ static void show_negated_gitcomp(const struct option *opts, int show_all,\n \t\tcase OPTION_NEGBIT:\n \t\tcase OPTION_COUNTUP:\n \t\tcase OPTION_SET_INT:\n+\t\tcase OPTION_SET_VALUE:\n \t\t\thas_unset_form = 1;\n \t\t\tbreak;\n \t\tdefault:\ndiff --git a/parse-options.h b/parse-options.h\nindex 57a7fe9d91..764e7f7896 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -20,6 +20,7 @@ enum parse_opt_type {\n \tOPTION_BITOP,\n \tOPTION_COUNTUP,\n \tOPTION_SET_INT,\n+\tOPTION_SET_VALUE,\n \t/* options with arguments (usually) */\n \tOPTION_STRING,\n \tOPTION_INTEGER,\n@@ -158,8 +159,34 @@ struct option {\n \tparse_opt_ll_cb *ll_callback;\n \tintptr_t extra;\n \tparse_opt_subcommand_fn *subcommand_fn;\n+\tintptr_t (*get_value)(void *);\n+\tvoid (*set_value)(void *, intptr_t);\n };\n\n+#define DEFINE_OPTION_VALUE_TYPE(type_name, type) \\\n+static inline intptr_t type_name##__get(void *void_ptr) \\\n+{ \\\n+\ttype *ptr = void_ptr; \\\n+\treturn (intptr_t)*ptr; \\\n+} \\\n+static inline void type_name##__set(void *void_ptr, intptr_t value) \\\n+{ \\\n+\ttype *ptr = void_ptr; \\\n+\t*ptr = (type)value; \\\n+} \\\n+static inline void *type_name##__check(type *ptr) \\\n+{ \\\n+\treturn ptr; \\\n+} \\\n+static inline void *type_name##__check(type *ptr)\n+\n+DEFINE_OPTION_VALUE_TYPE(int, int);\n+\n+#define OPTION_VALUE(type_name, v) \\\n+\t.get_value = type_name##__get, \\\n+\t.set_value = type_name##__set, \\\n+\t.value = (1 ? (v) : type_name##__check(v))\n+\n #define OPT_BIT_F(s, l, v, h, b, f) { \\\n \t.type = OPTION_BIT, \\\n \t.short_name = (s), \\\n@@ -256,15 +283,18 @@ struct option {\n \t.flags = PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, \\\n \t.defval = 1, \\\n }\n-#define OPT_CMDMODE_F(s, l, v, h, i, f) { \\\n-\t.type = OPTION_SET_INT, \\\n+\n+#define OPT_CMDMODE_T_F(s, l, t, v, h, i, f) { \\\n+\t.type = OPTION_SET_VALUE, \\\n \t.short_name = (s), \\\n \t.long_name = (l), \\\n-\t.value = (v), \\\n+\tOPTION_VALUE(t, (v)), \\\n \t.help = (h), \\\n \t.flags = PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG | (f), \\\n \t.defval = (i), \\\n }\n+#define OPT_CMDMODE_T(s, l, t, v, h, i) OPT_CMDMODE_T_F(s, l, t, v, h, i, 0)\n+#define OPT_CMDMODE_F(s, l, v, h, i, f) OPT_CMDMODE_T_F(s, l, int, v, h, i, f)\n #define OPT_CMDMODE(s, l, v, h, i) OPT_CMDMODE_F(s, l, v, h, i, 0)\n\n #define OPT_INTEGER(s, l, v, h) OPT_INTEGER_F(s, l, v, h, 0)\n--\n2.42.0\n\n"},{"id":"482566","messageId":"ZRvhEWHWn4nDynD0@ugly","threadId":"60216","inReplyTo":"d9defed8-4e7e-4b84-be3d-57155d973320@web.de","subject":"Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-10-03T09:38:25Z","receivedAt":"2023-10-03T09:38:31Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Tue, Oct 03, 2023 at 10:49:12AM +0200, René Scharfe wrote:\n>Am 21.09.23 um 12:40 schrieb Oswald Buddenhagen:\n>> On Wed, Sep 20, 2023 at 10:18:10AM +0200, René Scharfe wrote:\n>>> If we base it on type size then we're making assumptions that I\n>>> find hard to justify.\n>>>\n>> the only one i can think of is signedness. i think this can be safely\n>> ignored as long as we use only small positive integers.\n>\n>I don't fully understand the pointer-sign warning, so I'm not\n>confident enough to silence it.\n>\nin theory, differently signed integers may have completely different \nbinary representations. but afaik, that only ever mattered for negative \nnumbers. and c++20 actually codifies two's complement, which was the \nde-facto standard for decades already.\nso in practice it just means that we may be assigning a value that is \noutside the range of the actual type. but small positive values are \ncompatible between signed and unsiged types.\n\nregards\n"},{"id":"482575","messageId":"xmqq4jj71vbj.fsf@gitster.g","threadId":"60216","inReplyTo":"6cb09270-04b9-456e-8d7e-97137e56e9e2@web.de","subject":"Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-03T17:15:28Z","receivedAt":"2023-10-03T17:15:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> +DEFINE_OPTION_VALUE_TYPE(resume_type, enum resume_type);\n\nThese are a bit annoying, but because we need a token that can be ## pasted\nto form a valid identifier, we cannot help it.\n\n> diff --git a/parse-options.c b/parse-options.c\n> index e8e076c3a6..63a2247128 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -85,7 +85,7 @@ static enum parse_opt_result opt_command_mode_error(\n> \t\tif (that == opt ||\n> \t\t !(that->flags & PARSE_OPT_CMDMODE) ||\n> \t\t that->value != opt->value ||\n> -\t\t that->defval != *(int *)opt->value)\n> +\t\t that->defval != opt->get_value(opt->value))\n> \t\t\tcontinue;\n\nSo, instead of assuming the pointer stuffed in opt->value member can\nbe dereferenced as inteter pointer, we have the get_value method for\nthe option and invoke it to grab the value, and compare it with the\ndefault value.\n\n> @@ -122,7 +122,8 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n> \t * is not a grave error, so let it pass.\n> \t */\n> \tif ((opt->flags & PARSE_OPT_CMDMODE) &&\n> -\t *(int *)opt->value && *(int *)opt->value != opt->defval)\n> +\t opt->get_value(opt->value) &&\n> +\t opt->get_value(opt->value) != opt->defval)\n> \t\treturn opt_command_mode_error(opt, all_opts, flags);\n\nLikewise.\n\n> @@ -160,6 +161,10 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n> \t\t*(int *)opt->value = unset ? 0 : opt->defval;\n> \t\treturn 0;\n>\n> +\tcase OPTION_SET_VALUE:\n> +\t\topt->set_value(opt->value, unset ? 0 : opt->defval);\n> +\t\treturn 0;\n\nHere we see the previous way in the precontext of this hunk that is\nused for OPTION_SET_INT, but in the new type-safe-enum world order,\nthat uses OPTION_SET_VALUE, the set_value method should know what to\ndo with the pointer that is in opt->value.\n\n> diff --git a/parse-options.h b/parse-options.h\n> index 57a7fe9d91..764e7f7896 100644\n> --- a/parse-options.h\n> +++ b/parse-options.h\n> @@ -20,6 +20,7 @@ enum parse_opt_type {\n> \tOPTION_BITOP,\n> \tOPTION_COUNTUP,\n> \tOPTION_SET_INT,\n> +\tOPTION_SET_VALUE,\n> \t/* options with arguments (usually) */\n> \tOPTION_STRING,\n> \tOPTION_INTEGER,\n> @@ -158,8 +159,34 @@ struct option {\n> \tparse_opt_ll_cb *ll_callback;\n> \tintptr_t extra;\n> \tparse_opt_subcommand_fn *subcommand_fn;\n> +\tintptr_t (*get_value)(void *);\n> +\tvoid (*set_value)(void *, intptr_t);\n> };\n\nOK.\n\n> +#define DEFINE_OPTION_VALUE_TYPE(type_name, type) \\\n> +static inline intptr_t type_name##__get(void *void_ptr) \\\n> +{ \\\n> +\ttype *ptr = void_ptr; \\\n> +\treturn (intptr_t)*ptr; \\\n> +} \\\n> +static inline void type_name##__set(void *void_ptr, intptr_t value) \\\n> +{ \\\n> +\ttype *ptr = void_ptr; \\\n> +\t*ptr = (type)value; \\\n> +} \\\n> +static inline void *type_name##__check(type *ptr) \\\n> +{ \\\n> +\treturn ptr; \\\n> +} \\\n> +static inline void *type_name##__check(type *ptr)\n\nFun. So a typical pattern is that for \"enum foo\", the foo__get() is\ncreated from the above template and becomes the .get_value method.\n\nCopying from an earlier hunk, the get_value() method is used like\nso:\n\n> -\t\t that->defval != *(int *)opt->value)\n> +\t\t that->defval != opt->get_value(opt->value))\n\nWe pass opt->value (which is void *) to foo__get(), we have a local\nvariable \"enum foo *ptr\" and assign it in there, and dereference it.\nWe used to dereference the pointer as if it were a pointer to an\ninteger, so the type of foo__get() could be \"int\", but because we\ncompare it with the .defval member, which is of type \"intptr_t\", the\nreturn type of the get_value() method being \"intptr_t\" would make it\nconsistent here. I am not sure why defval need to be \"intptr_t\", and\nfor the purpose of this topic it would have been cleaner if it were\n\"int\", but that is a tangent (probably somebody uses it as the default\nvalue for a pointer variable and points it at some default object).\n\nThe setter is also reasonable. An earlier hunk used it like so:\n\n> +\t\topt->set_value(opt->value, unset ? 0 : opt->defval);\n\nopt->value which is (void *) is assigned to \"enum foo *ptr\", and\nusing that pointer, \"(enum foo)opt->defval\" (or 0) is assinged\nthere. Pretty straight-forward.\n\n> +DEFINE_OPTION_VALUE_TYPE(int, int);\n> +\n> +#define OPTION_VALUE(type_name, v) \\\n> +\t.get_value = type_name##__get, \\\n> +\t.set_value = type_name##__set, \\\n> +\t.value = (1 ? (v) : type_name##__check(v))\n\nThis is cute. foo__check() is declared to take \"enum foo *\" and\nreturns it as \"void *\", but because the condition to the ternary\noperator is constant \"true\", it is discarded. The only expected\neffect is to force the compiler to catch type errors when v is not\nof type \"enum foo *\".\n\nUnless it is \"void *\", I presume? Then foo__check() would be happy,\nbut typically OPTION_VALUE() is used as an implementation detail of\nOPT_CMDMODE_T() and you are expected to say something like \"&variable\"\nfor \"v\" above, so it would be OK (because you cannot have a variable\nof type \"void\").\n\nThanks for a fun read.\n\n"},{"id":"482579","messageId":"88cb2db8-e5cb-470a-8060-7a1b898c91f9@web.de","threadId":"60216","inReplyTo":"ZRvhEWHWn4nDynD0@ugly","subject":"Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-10-03T17:54:28Z","receivedAt":"2023-10-03T17:54:40Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.10.23 um 11:38 schrieb Oswald Buddenhagen:\n> On Tue, Oct 03, 2023 at 10:49:12AM +0200, René Scharfe wrote:\n>> Am 21.09.23 um 12:40 schrieb Oswald Buddenhagen:\n>>> On Wed, Sep 20, 2023 at 10:18:10AM +0200, René Scharfe wrote:\n>>>> If we base it on type size then we're making assumptions that\n>>>> I find hard to justify.\n>>>>\n>>> the only one i can think of is signedness. i think this can be\n>>> safely ignored as long as we use only small positive integers.\n>>\n>> I don't fully understand the pointer-sign warning, so I'm not\n>> confident enough to silence it.\n>>\n> in theory, differently signed integers may have completely different\n> binary representations. but afaik, that only ever mattered for\n> negative numbers. and c++20 actually codifies two's complement, which\n> was the de-facto standard for decades already. so in practice it just\n> means that we may be assigning a value that is outside the range of\n> the actual type. but small positive values are compatible between\n> signed and unsiged types.\n\nC++ is not relevant for Git, but C23 is going to to stop supporting\nbinary representations other than two's complement as well.\n\nStill I don't feel comfortable overriding compiler warnings for\nsomething pedestrian as a command line parser. No idea what other\nassumptions are made in compilers around enums. I'd rather honor\nthe warnings and avoid any forcing or trickery if possible. Or at\nleast leave that to more capable hands.\n\nRené\n"},{"id":"482583","messageId":"ZRxcZbH2u5Oa9WCi@ugly","threadId":"60216","inReplyTo":"88cb2db8-e5cb-470a-8060-7a1b898c91f9@web.de","subject":"Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-10-03T18:24:37Z","receivedAt":"2023-10-03T18:24:43Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Tue, Oct 03, 2023 at 07:54:28PM +0200, René Scharfe wrote:\n>C++ is not relevant for Git, but C23 is going to to stop supporting\n>binary representations other than two's complement as well.\n>\nit is relevant insofar as that every platform that comes with a recent \nc++ compiler uses two's complement. this then applies to any language, \nregardless of standard.\n\n>Still I don't feel comfortable overriding compiler warnings for\n>something pedestrian as a command line parser. No idea what other\n>assumptions are made in compilers around enums.\n>\ni'm not sure what you're really worried about. if there was an actual \nproblem, we'd have noticed by now. as far as i'm concerned, the question \nis only how to codify the status quo in the most elegant way. putting a \ntypecast into a macro certainly qualifies in my book.\n\nregards\n"}]}