* [PATCH 1/2] parse-options: add int value pointer to struct option
@ 2023-09-09 21:10 René Scharfe
2023-09-09 21:14 ` [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE René Scharfe
2023-09-10 18:40 ` [PATCH 1/2] parse-options: add int value pointer to struct option Taylor Blau
0 siblings, 2 replies; 30+ messages in thread
From: René Scharfe @ 2023-09-09 21:10 UTC (permalink / raw)
To: Git List; +Cc: Jeff King, Junio C Hamano
Add an int pointer, value_int, to struct option to provide a typed value
pointer for the various integer options. It allows type checks at
compile time, which is not possible with the void pointer, value. Its
use is optional for now.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
parse-options.c | 34 +++++++++++++++++++---------------
parse-options.h | 2 ++
2 files changed, 21 insertions(+), 15 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index e8e076c3a6..2552745804 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -82,10 +82,11 @@ static enum parse_opt_result opt_command_mode_error(
* already, and report that this is not compatible with it.
*/
for (that = all_opts; that->type != OPTION_END; that++) {
+ int *value_int = opt->value_int ? opt->value_int : opt->value;
if (that == opt ||
!(that->flags & PARSE_OPT_CMDMODE) ||
that->value != opt->value ||
- that->defval != *(int *)opt->value)
+ that->defval != *value_int)
continue;
if (that->long_name)
@@ -109,6 +110,7 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
const char *s, *arg;
const int unset = flags & OPT_UNSET;
int err;
+ int *value_int = opt->value_int ? opt->value_int : opt->value;
if (unset && p->opt)
return error(_("%s takes no value"), optname(opt, flags));
@@ -122,7 +124,7 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
* is not a grave error, so let it pass.
*/
if ((opt->flags & PARSE_OPT_CMDMODE) &&
- *(int *)opt->value && *(int *)opt->value != opt->defval)
+ *value_int && *value_int != opt->defval)
return opt_command_mode_error(opt, all_opts, flags);
switch (opt->type) {
@@ -131,33 +133,33 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
case OPTION_BIT:
if (unset)
- *(int *)opt->value &= ~opt->defval;
+ *value_int &= ~opt->defval;
else
- *(int *)opt->value |= opt->defval;
+ *value_int |= opt->defval;
return 0;
case OPTION_NEGBIT:
if (unset)
- *(int *)opt->value |= opt->defval;
+ *value_int |= opt->defval;
else
- *(int *)opt->value &= ~opt->defval;
+ *value_int &= ~opt->defval;
return 0;
case OPTION_BITOP:
if (unset)
BUG("BITOP can't have unset form");
- *(int *)opt->value &= ~opt->extra;
- *(int *)opt->value |= opt->defval;
+ *value_int &= ~opt->extra;
+ *value_int |= opt->defval;
return 0;
case OPTION_COUNTUP:
- if (*(int *)opt->value < 0)
- *(int *)opt->value = 0;
- *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
+ if (*value_int < 0)
+ *value_int = 0;
+ *value_int = unset ? 0 : *value_int + 1;
return 0;
case OPTION_SET_INT:
- *(int *)opt->value = unset ? 0 : opt->defval;
+ *value_int = unset ? 0 : opt->defval;
return 0;
case OPTION_STRING:
@@ -206,11 +208,11 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
}
case OPTION_INTEGER:
if (unset) {
- *(int *)opt->value = 0;
+ *value_int = 0;
return 0;
}
if (opt->flags & PARSE_OPT_OPTARG && !p->opt) {
- *(int *)opt->value = opt->defval;
+ *value_int = opt->defval;
return 0;
}
if (get_arg(p, opt, flags, &arg))
@@ -218,7 +220,7 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
if (!*arg)
return error(_("%s expects a numerical value"),
optname(opt, flags));
- *(int *)opt->value = strtol(arg, (char **)&s, 10);
+ *value_int = strtol(arg, (char **)&s, 10);
if (*s)
return error(_("%s expects a numerical value"),
optname(opt, flags));
@@ -483,6 +485,8 @@ static void parse_options_check(const struct option *opts)
if (opts->type == OPTION_SET_INT && !opts->defval &&
opts->long_name && !(opts->flags & PARSE_OPT_NONEG))
optbug(opts, "OPTION_SET_INT 0 should not be negatable");
+ if (opts->value && opts->value_int)
+ optbug(opts, "only a single value type supported");
switch (opts->type) {
case OPTION_COUNTUP:
case OPTION_BIT:
diff --git a/parse-options.h b/parse-options.h
index 57a7fe9d91..5e7475bd2d 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -158,6 +158,8 @@ struct option {
parse_opt_ll_cb *ll_callback;
intptr_t extra;
parse_opt_subcommand_fn *subcommand_fn;
+
+ int *value_int;
};
#define OPT_BIT_F(s, l, v, h, b, f) { \
--
2.42.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
2023-09-09 21:10 [PATCH 1/2] parse-options: add int value pointer to struct option René Scharfe
@ 2023-09-09 21:14 ` René Scharfe
2023-09-10 10:18 ` Oswald Buddenhagen
` (2 more replies)
2023-09-10 18:40 ` [PATCH 1/2] parse-options: add int value pointer to struct option Taylor Blau
1 sibling, 3 replies; 30+ messages in thread
From: René Scharfe @ 2023-09-09 21:14 UTC (permalink / raw)
To: Git List; +Cc: Jeff King, Junio C Hamano
Some uses of OPT_CMDMODE provide a pointer to an enum. It is
dereferenced as an int pointer in parse-options.c::get_value(). These
two types are incompatible, though -- the storage size of an enum can
vary between platforms. C23 would allow us to specify the underlying
type of the different enums, making them compatible, but with C99 the
easiest safe option is to actually use int as the value type.
Convert the offending OPT_CMDMODE users and use the typed value_int
point in the macro's definition to enforce that type for future ones.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
builtin/am.c | 2 +-
builtin/help.c | 5 +++--
builtin/ls-tree.c | 2 +-
builtin/rebase.c | 2 +-
builtin/replace.c | 3 ++-
builtin/stripspace.c | 2 +-
parse-options.h | 2 +-
7 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/builtin/am.c b/builtin/am.c
index 202040b62e..ebb72ebaaa 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2269,7 +2269,7 @@ enum resume_type {
};
struct resume_mode {
- enum resume_type mode;
+ int mode;
enum show_patch_type sub_mode;
};
diff --git a/builtin/help.c b/builtin/help.c
index dc1fbe2b98..e8aedb932c 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -42,7 +42,7 @@ enum show_config_type {
SHOW_CONFIG_SECTIONS,
};
-static enum help_action {
+enum help_action {
HELP_ACTION_ALL = 1,
HELP_ACTION_GUIDES,
HELP_ACTION_CONFIG,
@@ -50,7 +50,8 @@ static enum help_action {
HELP_ACTION_DEVELOPER_INTERFACES,
HELP_ACTION_CONFIG_FOR_COMPLETION,
HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION,
-} cmd_mode;
+};
+static int cmd_mode;
static const char *html_path;
static int verbose = 1;
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 209d2dc0d5..6f8c43f729 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -346,7 +346,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
int i, full_tree = 0;
int full_name = !prefix || !*prefix;
read_tree_fn_t fn = NULL;
- enum ls_tree_cmdmode cmdmode = MODE_DEFAULT;
+ int cmdmode = MODE_DEFAULT;
int null_termination = 0;
struct ls_tree_options options = { 0 };
const struct option ls_tree_options[] = {
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 50cb85751f..d11e749579 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -111,7 +111,7 @@ struct rebase_options {
REBASE_INTERACTIVE_EXPLICIT = 1<<4,
} flags;
struct strvec git_am_opts;
- enum action action;
+ int action;
char *reflog_action;
int signoff;
int allow_rerere_autoupdate;
diff --git a/builtin/replace.c b/builtin/replace.c
index da59600ad2..d0063d3feb 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -553,7 +553,8 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
MODE_GRAFT,
MODE_CONVERT_GRAFT_FILE,
MODE_REPLACE
- } cmdmode = MODE_UNSPECIFIED;
+ };
+ int cmdmode = MODE_UNSPECIFIED;
struct option options[] = {
OPT_CMDMODE('l', "list", &cmdmode, N_("list replace refs"), MODE_LIST),
OPT_CMDMODE('d', "delete", &cmdmode, N_("delete replace refs"), MODE_DELETE),
diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index 7b700a9fb1..f6de0b17dc 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -32,7 +32,7 @@ enum stripspace_mode {
int cmd_stripspace(int argc, const char **argv, const char *prefix)
{
struct strbuf buf = STRBUF_INIT;
- enum stripspace_mode mode = STRIP_DEFAULT;
+ int mode = STRIP_DEFAULT;
int nongit;
const struct option options[] = {
diff --git a/parse-options.h b/parse-options.h
index 5e7475bd2d..349c3fca04 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -262,7 +262,7 @@ struct option {
.type = OPTION_SET_INT, \
.short_name = (s), \
.long_name = (l), \
- .value = (v), \
+ .value_int = (v), \
.help = (h), \
.flags = PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG | (f), \
.defval = (i), \
--
2.42.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
2023-09-09 21:14 ` [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE René Scharfe
@ 2023-09-10 10:18 ` Oswald Buddenhagen
2023-09-11 20:11 ` René Scharfe
2023-09-11 19:12 ` Junio C Hamano
2023-09-19 9:40 ` Oswald Buddenhagen
2 siblings, 1 reply; 30+ messages in thread
From: Oswald Buddenhagen @ 2023-09-10 10:18 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Jeff King, Junio C Hamano
On Sat, Sep 09, 2023 at 11:14:20PM +0200, René Scharfe wrote:
>Convert the offending OPT_CMDMODE users and use the typed value_int
>point in the macro's definition to enforce that type for future ones.
>
that defeats -Wswitch[-enum], though.
the pedantically correct solution would be using setter callbacks.
regards
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] parse-options: add int value pointer to struct option
2023-09-09 21:10 [PATCH 1/2] parse-options: add int value pointer to struct option René Scharfe
2023-09-09 21:14 ` [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE René Scharfe
@ 2023-09-10 18:40 ` Taylor Blau
2023-09-11 19:19 ` Junio C Hamano
2023-09-11 20:12 ` René Scharfe
1 sibling, 2 replies; 30+ messages in thread
From: Taylor Blau @ 2023-09-10 18:40 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Jeff King, Junio C Hamano
On Sat, Sep 09, 2023 at 11:10:36PM +0200, René Scharfe wrote:
> Add an int pointer, value_int, to struct option to provide a typed value
> pointer for the various integer options. It allows type checks at
> compile time, which is not possible with the void pointer, value. Its
> use is optional for now.
This is an interesting direction. I wonder about whether or not you'd
consider changing the option structure to contain a tagged union type
that represents some common cases we'd want from a parse-options
callback, something like:
struct option {
/* ... */
union {
void *value;
int *value_int;
/* etc ... */
} u;
enum option_type t;
};
where option_type has some value corresponding to "void *", another for
"int *", and so on.
Alternatively, perhaps you are thinking that we'd use both the value
pointer and the value_int pointer to point at potentially different
values in the same callback. I don't have strong feelings about it, but
I'd just as soon encourage us to shy away from that approach, since
assigning a single callback parameter to each function seems more
organized.
> @@ -109,6 +110,7 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
> const char *s, *arg;
> const int unset = flags & OPT_UNSET;
> int err;
> + int *value_int = opt->value_int ? opt->value_int : opt->value;
>
> if (unset && p->opt)
> return error(_("%s takes no value"), optname(opt, flags));
Reading this hunk, I wonder whether we even need a type tag (the
option_type enum above) if each callback knows a priori what type it
expects. But I think storing them together in a union makes sense to do.
Thanks,
Taylor
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
2023-09-09 21:14 ` [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE René Scharfe
2023-09-10 10:18 ` Oswald Buddenhagen
@ 2023-09-11 19:12 ` Junio C Hamano
2023-09-11 20:11 ` René Scharfe
2023-09-19 9:40 ` Oswald Buddenhagen
2 siblings, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2023-09-11 19:12 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Jeff King
René Scharfe <l.s.r@web.de> writes:
> Some uses of OPT_CMDMODE provide a pointer to an enum. It is
> dereferenced as an int pointer in parse-options.c::get_value(). These
> two types are incompatible, though -- the storage size of an enum can
> vary between platforms. C23 would allow us to specify the underlying
> type of the different enums, making them compatible, but with C99 the
> easiest safe option is to actually use int as the value type.
>
> Convert the offending OPT_CMDMODE users and use the typed value_int
> point in the macro's definition to enforce that type for future ones.
Interesting. I wondered if this means that applying [1/2] alone
will immediately break these places that [2/2] fixes, but the answer
is no, as the previous step did not make these places use the typed
pointer. But it also means that with this step alone to use "int",
instead of various "enum" types that can have representations that
are different from "int", would already "fix" the current code
while still casing back and forth from (void *)?
In any case, the two-patch series looks good, and it does not break
bisectability, either.
Thanks.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] parse-options: add int value pointer to struct option
2023-09-10 18:40 ` [PATCH 1/2] parse-options: add int value pointer to struct option Taylor Blau
@ 2023-09-11 19:19 ` Junio C Hamano
2023-09-11 22:28 ` Oswald Buddenhagen
2023-09-18 9:53 ` René Scharfe
2023-09-11 20:12 ` René Scharfe
1 sibling, 2 replies; 30+ messages in thread
From: Junio C Hamano @ 2023-09-11 19:19 UTC (permalink / raw)
To: Taylor Blau; +Cc: René Scharfe, Git List, Jeff King
Taylor Blau <me@ttaylorr.com> writes:
> callback, something like:
>
> struct option {
> /* ... */
> union {
> void *value;
> int *value_int;
> /* etc ... */
> } u;
> enum option_type t;
> };
>
> where option_type has some value corresponding to "void *", another for
> "int *", and so on.
Yup, that does cross my mind, even though I would have used
union {
void *void_ptr;
int *int_ptr;
} value;
or something without a rather meaningless 'u'.
> Alternatively, perhaps you are thinking that we'd use both the value
> pointer and the value_int pointer to point at potentially different
> values in the same callback. I don't have strong feelings about it, but
> I'd just as soon encourage us to shy away from that approach, since
> assigning a single callback parameter to each function seems more
> organized.
We have seen (with Peff's "-Wunused" work) that there are small
number of cases that it would be handy for a callback to be told the
locations of multiple external variables, but I do not think it
would be a good solution to that problem to have "void *value" and
"int value_int" next to each other and allow them to coexist, as it
would work only when these multiple variables happen to be of the
right types.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
2023-09-11 19:12 ` Junio C Hamano
@ 2023-09-11 20:11 ` René Scharfe
0 siblings, 0 replies; 30+ messages in thread
From: René Scharfe @ 2023-09-11 20:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Jeff King
Am 11.09.23 um 21:12 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> Some uses of OPT_CMDMODE provide a pointer to an enum. It is
>> dereferenced as an int pointer in parse-options.c::get_value(). These
>> two types are incompatible, though -- the storage size of an enum can
>> vary between platforms. C23 would allow us to specify the underlying
>> type of the different enums, making them compatible, but with C99 the
>> easiest safe option is to actually use int as the value type.
>>
>> Convert the offending OPT_CMDMODE users and use the typed value_int
>> point in the macro's definition to enforce that type for future ones.
>
> Interesting. I wondered if this means that applying [1/2] alone
> will immediately break these places that [2/2] fixes, but the answer
> is no, as the previous step did not make these places use the typed
> pointer. But it also means that with this step alone to use "int",
> instead of various "enum" types that can have representations that
> are different from "int", would already "fix" the current code
> while still casing back and forth from (void *)?
Yes and yes. And the change to use value_int on its own makes the type
mismatch visible via compiler warnings. It guards against future
violations.
René
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
2023-09-10 10:18 ` Oswald Buddenhagen
@ 2023-09-11 20:11 ` René Scharfe
2023-09-12 8:40 ` Jeff King
0 siblings, 1 reply; 30+ messages in thread
From: René Scharfe @ 2023-09-11 20:11 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: Git List, Jeff King, Junio C Hamano
Am 10.09.23 um 12:18 schrieb Oswald Buddenhagen:
> On Sat, Sep 09, 2023 at 11:14:20PM +0200, René Scharfe wrote:
>> Convert the offending OPT_CMDMODE users and use the typed value_int
>> point in the macro's definition to enforce that type for future ones.
>>
> that defeats -Wswitch[-enum], though.
True. Though I don't fully understand these warnings (why not then
also warn about if without else?), but taking them away is a bit rude
to those who care.
> the pedantically correct solution would be using setter callbacks.
Or to use an int to point to and then copy into a companion enum
variable to after parsing, which would be my choice.
René
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] parse-options: add int value pointer to struct option
2023-09-10 18:40 ` [PATCH 1/2] parse-options: add int value pointer to struct option Taylor Blau
2023-09-11 19:19 ` Junio C Hamano
@ 2023-09-11 20:12 ` René Scharfe
1 sibling, 0 replies; 30+ messages in thread
From: René Scharfe @ 2023-09-11 20:12 UTC (permalink / raw)
To: Taylor Blau; +Cc: Git List, Jeff King, Junio C Hamano
Am 10.09.23 um 20:40 schrieb Taylor Blau:
> On Sat, Sep 09, 2023 at 11:10:36PM +0200, René Scharfe wrote:
>> Add an int pointer, value_int, to struct option to provide a typed value
>> pointer for the various integer options. It allows type checks at
>> compile time, which is not possible with the void pointer, value. Its
>> use is optional for now.
>
> This is an interesting direction. I wonder about whether or not you'd
> consider changing the option structure to contain a tagged union type
> that represents some common cases we'd want from a parse-options
> callback, something like:
>
> struct option {
> /* ... */
> union {
> void *value;
> int *value_int;
> /* etc ... */
> } u;
> enum option_type t;
> };
>
> where option_type has some value corresponding to "void *", another for
> "int *", and so on.
In a hand-made struct option this would only provide a very limited form
of type safety. It reduces the number of incorrect types to choose from
from basically infinity to a handful, but still allows pointing the
union e.g. to an int for an option that takes a long or a string without
any compiler warning or error.
Convenience macros like OPT_CMDMODE could use the union to provide a
type safe interface, though, true. This might suffice for our purposes.
> Alternatively, perhaps you are thinking that we'd use both the value
> pointer and the value_int pointer to point at potentially different
> values in the same callback. I don't have strong feelings about it, but
> I'd just as soon encourage us to shy away from that approach, since
> assigning a single callback parameter to each function seems more
> organized.
Right, we only need one active value pointer per option.
>> @@ -109,6 +110,7 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
>> const char *s, *arg;
>> const int unset = flags & OPT_UNSET;
>> int err;
>> + int *value_int = opt->value_int ? opt->value_int : opt->value;
>>
>> if (unset && p->opt)
>> return error(_("%s takes no value"), optname(opt, flags));
>
> Reading this hunk, I wonder whether we even need a type tag (the
> option_type enum above) if each callback knows a priori what type it
> expects. But I think storing them together in a union makes sense to do.
Yes, option types (OPTION_INTEGER etc.) already imply a pointer type,
no additional tag needed.
René
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] parse-options: add int value pointer to struct option
2023-09-11 19:19 ` Junio C Hamano
@ 2023-09-11 22:28 ` Oswald Buddenhagen
2023-09-18 11:34 ` Kristoffer Haugsbakk
2023-09-18 9:53 ` René Scharfe
1 sibling, 1 reply; 30+ messages in thread
From: Oswald Buddenhagen @ 2023-09-11 22:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Taylor Blau, René Scharfe, Git List, Jeff King
On Mon, Sep 11, 2023 at 12:19:17PM -0700, Junio C Hamano wrote:
>Yup, that does cross my mind, even though I would have used
>
> union {
> void *void_ptr;
> int *int_ptr;
> } value;
>
>or something without a rather meaningless 'u'.
>
i'd go the opposite way and make it an anonymous union.
that would require c11, though. imo not exactly an outrageous
proposition in 2023, but ...
regards
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
2023-09-11 20:11 ` René Scharfe
@ 2023-09-12 8:40 ` Jeff King
2023-09-16 17:45 ` Junio C Hamano
0 siblings, 1 reply; 30+ messages in thread
From: Jeff King @ 2023-09-12 8:40 UTC (permalink / raw)
To: René Scharfe; +Cc: Oswald Buddenhagen, Git List, Junio C Hamano
On Mon, Sep 11, 2023 at 10:11:56PM +0200, René Scharfe wrote:
> Am 10.09.23 um 12:18 schrieb Oswald Buddenhagen:
> > On Sat, Sep 09, 2023 at 11:14:20PM +0200, René Scharfe wrote:
> >> Convert the offending OPT_CMDMODE users and use the typed value_int
> >> point in the macro's definition to enforce that type for future ones.
> >>
> > that defeats -Wswitch[-enum], though.
>
> True. Though I don't fully understand these warnings (why not then
> also warn about if without else?), but taking them away is a bit rude
> to those who care.
I think losing warnings is unfortunate, but it's just one example.
We're losing the type information completely from the values. That might
be of use to the compiler (both for -Wswitch, but also for code
generation in general). But it is also of use to human readers, who see
that "foo" is of type "enum bar" and know what it's supposed to contain.
> > the pedantically correct solution would be using setter callbacks.
>
> Or to use an int to point to and then copy into a companion enum
> variable to after parsing, which would be my choice.
Yeah, I had the same thought. I'm just not sure how to do that in a way
that isn't a pain for the callers.
-Peff
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
2023-09-12 8:40 ` Jeff King
@ 2023-09-16 17:45 ` Junio C Hamano
2023-09-18 9:28 ` René Scharfe
0 siblings, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2023-09-16 17:45 UTC (permalink / raw)
To: René Scharfe; +Cc: Jeff King, Oswald Buddenhagen, Git List
Jeff King <peff@peff.net> writes:
>> True. Though I don't fully understand these warnings (why not then
>> also warn about if without else?), but taking them away is a bit rude
>> to those who care.
>
> I think losing warnings is unfortunate, but it's just one example.
> We're losing the type information completely from the values.
> ...
>> Or to use an int to point to and then copy into a companion enum
>> variable to after parsing, which would be my choice.
>
> Yeah, I had the same thought. I'm just not sure how to do that in a way
> that isn't a pain for the callers.
The discussion seems to have petered out around this point.
What (if anything) do we want to do with this topic?
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
2023-09-16 17:45 ` Junio C Hamano
@ 2023-09-18 9:28 ` René Scharfe
2023-09-18 10:10 ` Oswald Buddenhagen
2023-09-18 13:33 ` [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE Phillip Wood
0 siblings, 2 replies; 30+ messages in thread
From: René Scharfe @ 2023-09-18 9:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Oswald Buddenhagen, Git List
Am 16.09.23 um 19:45 schrieb Junio C Hamano:
> Jeff King <peff@peff.net> writes:
>
>>> True. Though I don't fully understand these warnings (why not then
>>> also warn about if without else?), but taking them away is a bit rude
>>> to those who care.
>>
>> I think losing warnings is unfortunate, but it's just one example.
>> We're losing the type information completely from the values.
>> ...
>>> Or to use an int to point to and then copy into a companion enum
>>> variable to after parsing, which would be my choice.
>>
>> Yeah, I had the same thought. I'm just not sure how to do that in a way
>> that isn't a pain for the callers.
>
> The discussion seems to have petered out around this point.
> What (if anything) do we want to do with this topic?
Here's a version that preserves the enums by using additional int
variables just for the parsing phase. No tricks. The diff is long, but
most changes aren't particularly complicated and the resulting code is
not that ugly. Except for builtin/am.c perhaps, which changes the
command mode value using a callback as well.
--- >8 ---
Subject: [PATCH v2 2/2] parse-options: use and require int pointer for OPT_CMDMODE
Some uses of OPT_CMDMODE provide a pointer to an enum. It is
dereferenced as an int pointer in parse-options.c::get_value(). The two
types are incompatible, though -- the storage size of an enum can vary
between platforms. C23 would allow us to specify the underlying type of
the diffenrent enums, making them compatible, but with C99 the easiest
safe option is to actually use int as the value type.
Convert the offending OPT_CMDMODE users to point to a new int value and
set the enum value after parsing. Switch to the typed value_int pointer
in the macro's definition to enforce that type for future users.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
builtin/am.c | 24 +++++++++++++-----------
builtin/help.c | 16 +++++++++-------
builtin/ls-tree.c | 12 +++++++-----
builtin/rebase.c | 15 +++++++++------
builtin/replace.c | 12 +++++++-----
builtin/stripspace.c | 6 ++++--
parse-options.h | 2 +-
7 files changed, 50 insertions(+), 37 deletions(-)
diff --git a/builtin/am.c b/builtin/am.c
index 202040b62e..00930e2152 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2270,13 +2270,14 @@ enum resume_type {
struct resume_mode {
enum resume_type mode;
+ int mode_int;
enum show_patch_type sub_mode;
};
static int parse_opt_show_current_patch(const struct option *opt, const char *arg, int unset)
{
int *opt_value = opt->value;
- struct resume_mode *resume = container_of(opt_value, struct resume_mode, mode);
+ struct resume_mode *resume = container_of(opt_value, struct resume_mode, mode_int);
/*
* Please update $__git_showcurrentpatch in git-completion.bash
@@ -2300,12 +2301,12 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar
"--show-current-patch", arg);
}
- if (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
+ if (resume->mode_int == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
return error(_("options '%s=%s' and '%s=%s' "
"cannot be used together"),
"--show-current-patch", "--show-current-patch", arg, valid_modes[resume->sub_mode]);
- resume->mode = RESUME_SHOW_PATCH;
+ resume->mode_int = RESUME_SHOW_PATCH;
resume->sub_mode = new_value;
return 0;
}
@@ -2316,7 +2317,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)
int binary = -1;
int keep_cr = -1;
int patch_format = PATCH_FORMAT_UNKNOWN;
- struct resume_mode resume = { .mode = RESUME_FALSE };
+ struct resume_mode resume = { .mode_int = RESUME_FALSE };
int in_progress;
int ret = 0;
@@ -2387,27 +2388,27 @@ int cmd_am(int argc, const char **argv, const char *prefix)
PARSE_OPT_NOARG),
OPT_STRING(0, "resolvemsg", &state.resolvemsg, NULL,
N_("override error message when patch failure occurs")),
- OPT_CMDMODE(0, "continue", &resume.mode,
+ OPT_CMDMODE(0, "continue", &resume.mode_int,
N_("continue applying patches after resolving a conflict"),
RESUME_RESOLVED),
- OPT_CMDMODE('r', "resolved", &resume.mode,
+ OPT_CMDMODE('r', "resolved", &resume.mode_int,
N_("synonyms for --continue"),
RESUME_RESOLVED),
- OPT_CMDMODE(0, "skip", &resume.mode,
+ OPT_CMDMODE(0, "skip", &resume.mode_int,
N_("skip the current patch"),
RESUME_SKIP),
- OPT_CMDMODE(0, "abort", &resume.mode,
+ OPT_CMDMODE(0, "abort", &resume.mode_int,
N_("restore the original branch and abort the patching operation"),
RESUME_ABORT),
- OPT_CMDMODE(0, "quit", &resume.mode,
+ OPT_CMDMODE(0, "quit", &resume.mode_int,
N_("abort the patching operation but keep HEAD where it is"),
RESUME_QUIT),
- { OPTION_CALLBACK, 0, "show-current-patch", &resume.mode,
+ { OPTION_CALLBACK, 0, "show-current-patch", &resume.mode_int,
"(diff|raw)",
N_("show the patch being applied"),
PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
parse_opt_show_current_patch, RESUME_SHOW_PATCH },
- OPT_CMDMODE(0, "allow-empty", &resume.mode,
+ OPT_CMDMODE(0, "allow-empty", &resume.mode_int,
N_("record the empty patch as an empty commit"),
RESUME_ALLOW_EMPTY),
OPT_BOOL(0, "committer-date-is-author-date",
@@ -2439,6 +2440,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)
am_load(&state);
argc = parse_options(argc, argv, prefix, options, usage, 0);
+ resume.mode = resume.mode_int;
if (binary >= 0)
fprintf_ln(stderr, _("The -b/--binary option has been a no-op for long time, and\n"
diff --git a/builtin/help.c b/builtin/help.c
index dc1fbe2b98..d76f88d544 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -51,6 +51,7 @@ static enum help_action {
HELP_ACTION_CONFIG_FOR_COMPLETION,
HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION,
} cmd_mode;
+static int cmd_mode_int;
static const char *html_path;
static int verbose = 1;
@@ -59,7 +60,7 @@ static int exclude_guides;
static int show_external_commands = -1;
static int show_aliases = -1;
static struct option builtin_help_options[] = {
- OPT_CMDMODE('a', "all", &cmd_mode, N_("print all available commands"),
+ OPT_CMDMODE('a', "all", &cmd_mode_int, N_("print all available commands"),
HELP_ACTION_ALL),
OPT_BOOL(0, "external-commands", &show_external_commands,
N_("show external commands in --all")),
@@ -72,19 +73,19 @@ static struct option builtin_help_options[] = {
HELP_FORMAT_INFO),
OPT__VERBOSE(&verbose, N_("print command description")),
- OPT_CMDMODE('g', "guides", &cmd_mode, N_("print list of useful guides"),
+ OPT_CMDMODE('g', "guides", &cmd_mode_int, N_("print list of useful guides"),
HELP_ACTION_GUIDES),
- OPT_CMDMODE(0, "user-interfaces", &cmd_mode,
+ OPT_CMDMODE(0, "user-interfaces", &cmd_mode_int,
N_("print list of user-facing repository, command and file interfaces"),
HELP_ACTION_USER_INTERFACES),
- OPT_CMDMODE(0, "developer-interfaces", &cmd_mode,
+ OPT_CMDMODE(0, "developer-interfaces", &cmd_mode_int,
N_("print list of file formats, protocols and other developer interfaces"),
HELP_ACTION_DEVELOPER_INTERFACES),
- OPT_CMDMODE('c', "config", &cmd_mode, N_("print all configuration variable names"),
+ OPT_CMDMODE('c', "config", &cmd_mode_int, N_("print all configuration variable names"),
HELP_ACTION_CONFIG),
- OPT_CMDMODE_F(0, "config-for-completion", &cmd_mode, "",
+ OPT_CMDMODE_F(0, "config-for-completion", &cmd_mode_int, "",
HELP_ACTION_CONFIG_FOR_COMPLETION, PARSE_OPT_HIDDEN),
- OPT_CMDMODE_F(0, "config-sections-for-completion", &cmd_mode, "",
+ OPT_CMDMODE_F(0, "config-sections-for-completion", &cmd_mode_int, "",
HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION, PARSE_OPT_HIDDEN),
OPT_END(),
@@ -640,6 +641,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, builtin_help_options,
builtin_help_usage, 0);
parsed_help_format = help_format;
+ cmd_mode = cmd_mode_int;
if (cmd_mode != HELP_ACTION_ALL &&
(show_external_commands >= 0 ||
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 209d2dc0d5..c64b38614a 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -346,7 +346,8 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
int i, full_tree = 0;
int full_name = !prefix || !*prefix;
read_tree_fn_t fn = NULL;
- enum ls_tree_cmdmode cmdmode = MODE_DEFAULT;
+ enum ls_tree_cmdmode cmdmode;
+ int cmdmode_int = MODE_DEFAULT;
int null_termination = 0;
struct ls_tree_options options = { 0 };
const struct option ls_tree_options[] = {
@@ -358,13 +359,13 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
LS_SHOW_TREES),
OPT_BOOL('z', NULL, &null_termination,
N_("terminate entries with NUL byte")),
- OPT_CMDMODE('l', "long", &cmdmode, N_("include object size"),
+ OPT_CMDMODE('l', "long", &cmdmode_int, N_("include object size"),
MODE_LONG),
- OPT_CMDMODE(0, "name-only", &cmdmode, N_("list only filenames"),
+ OPT_CMDMODE(0, "name-only", &cmdmode_int, N_("list only filenames"),
MODE_NAME_ONLY),
- OPT_CMDMODE(0, "name-status", &cmdmode, N_("list only filenames"),
+ OPT_CMDMODE(0, "name-status", &cmdmode_int, N_("list only filenames"),
MODE_NAME_STATUS),
- OPT_CMDMODE(0, "object-only", &cmdmode, N_("list only objects"),
+ OPT_CMDMODE(0, "object-only", &cmdmode_int, N_("list only objects"),
MODE_OBJECT_ONLY),
OPT_BOOL(0, "full-name", &full_name, N_("use full path names")),
OPT_BOOL(0, "full-tree", &full_tree,
@@ -384,6 +385,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, ls_tree_options,
ls_tree_usage, 0);
options.null_termination = null_termination;
+ cmdmode = cmdmode_int;
if (full_tree)
prefix = NULL;
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 50cb85751f..6dbe57f0ac 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1061,6 +1061,7 @@ static int check_exec_cmd(const char *cmd)
int cmd_rebase(int argc, const char **argv, const char *prefix)
{
struct rebase_options options = REBASE_OPTIONS_INIT;
+ int action;
const char *branch_name;
int ret, flags, total_argc, in_progress = 0;
int keep_base = 0;
@@ -1116,18 +1117,18 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
OPT_BIT(0, "no-ff", &options.flags,
N_("cherry-pick all commits, even if unchanged"),
REBASE_FORCE),
- OPT_CMDMODE(0, "continue", &options.action, N_("continue"),
+ OPT_CMDMODE(0, "continue", &action, N_("continue"),
ACTION_CONTINUE),
- OPT_CMDMODE(0, "skip", &options.action,
+ OPT_CMDMODE(0, "skip", &action,
N_("skip current patch and continue"), ACTION_SKIP),
- OPT_CMDMODE(0, "abort", &options.action,
+ OPT_CMDMODE(0, "abort", &action,
N_("abort and check out the original branch"),
ACTION_ABORT),
- OPT_CMDMODE(0, "quit", &options.action,
+ OPT_CMDMODE(0, "quit", &action,
N_("abort but keep HEAD where it is"), ACTION_QUIT),
- OPT_CMDMODE(0, "edit-todo", &options.action, N_("edit the todo list "
+ OPT_CMDMODE(0, "edit-todo", &action, N_("edit the todo list "
"during an interactive rebase"), ACTION_EDIT_TODO),
- OPT_CMDMODE(0, "show-current-patch", &options.action,
+ OPT_CMDMODE(0, "show-current-patch", &action,
N_("show the patch file being applied or merged"),
ACTION_SHOW_CURRENT_PATCH),
OPT_CALLBACK_F(0, "apply", &options, NULL,
@@ -1233,10 +1234,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
if (options.type != REBASE_UNSPECIFIED)
in_progress = 1;
+ action = options.action;
total_argc = argc;
argc = parse_options(argc, argv, prefix,
builtin_rebase_options,
builtin_rebase_usage, 0);
+ options.action = action;
if (preserve_merges_selected)
die(_("--preserve-merges was replaced by --rebase-merges\n"
diff --git a/builtin/replace.c b/builtin/replace.c
index da59600ad2..205c337ad3 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -554,12 +554,13 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
MODE_CONVERT_GRAFT_FILE,
MODE_REPLACE
} cmdmode = MODE_UNSPECIFIED;
+ int cmdmode_int = cmdmode;
struct option options[] = {
- OPT_CMDMODE('l', "list", &cmdmode, N_("list replace refs"), MODE_LIST),
- OPT_CMDMODE('d', "delete", &cmdmode, N_("delete replace refs"), MODE_DELETE),
- OPT_CMDMODE('e', "edit", &cmdmode, N_("edit existing object"), MODE_EDIT),
- OPT_CMDMODE('g', "graft", &cmdmode, N_("change a commit's parents"), MODE_GRAFT),
- OPT_CMDMODE(0, "convert-graft-file", &cmdmode, N_("convert existing graft file"), MODE_CONVERT_GRAFT_FILE),
+ OPT_CMDMODE('l', "list", &cmdmode_int, N_("list replace refs"), MODE_LIST),
+ OPT_CMDMODE('d', "delete", &cmdmode_int, N_("delete replace refs"), MODE_DELETE),
+ OPT_CMDMODE('e', "edit", &cmdmode_int, N_("edit existing object"), MODE_EDIT),
+ OPT_CMDMODE('g', "graft", &cmdmode_int, N_("change a commit's parents"), MODE_GRAFT),
+ OPT_CMDMODE(0, "convert-graft-file", &cmdmode_int, N_("convert existing graft file"), MODE_CONVERT_GRAFT_FILE),
OPT_BOOL_F('f', "force", &force, N_("replace the ref if it exists"),
PARSE_OPT_NOCOMPLETE),
OPT_BOOL(0, "raw", &raw, N_("do not pretty-print contents for --edit")),
@@ -572,6 +573,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, options, git_replace_usage, 0);
+ cmdmode = cmdmode_int;
if (!cmdmode)
cmdmode = argc ? MODE_REPLACE : MODE_LIST;
diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index 7b700a9fb1..e8efa0e7ac 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -33,13 +33,14 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
{
struct strbuf buf = STRBUF_INIT;
enum stripspace_mode mode = STRIP_DEFAULT;
+ int mode_int = mode;
int nongit;
const struct option options[] = {
- OPT_CMDMODE('s', "strip-comments", &mode,
+ OPT_CMDMODE('s', "strip-comments", &mode_int,
N_("skip and remove all lines starting with comment character"),
STRIP_COMMENTS),
- OPT_CMDMODE('c', "comment-lines", &mode,
+ OPT_CMDMODE('c', "comment-lines", &mode_int,
N_("prepend comment character and space to each line"),
COMMENT_LINES),
OPT_END()
@@ -49,6 +50,7 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
if (argc)
usage_with_options(stripspace_usage, options);
+ mode = mode_int;
if (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {
setup_git_directory_gently(&nongit);
git_config(git_default_config, NULL);
diff --git a/parse-options.h b/parse-options.h
index 5e7475bd2d..349c3fca04 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -262,7 +262,7 @@ struct option {
.type = OPTION_SET_INT, \
.short_name = (s), \
.long_name = (l), \
- .value = (v), \
+ .value_int = (v), \
.help = (h), \
.flags = PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG | (f), \
.defval = (i), \
--
2.42.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] parse-options: add int value pointer to struct option
2023-09-11 19:19 ` Junio C Hamano
2023-09-11 22:28 ` Oswald Buddenhagen
@ 2023-09-18 9:53 ` René Scharfe
2023-09-18 10:28 ` Oswald Buddenhagen
2023-09-18 16:17 ` Junio C Hamano
1 sibling, 2 replies; 30+ messages in thread
From: René Scharfe @ 2023-09-18 9:53 UTC (permalink / raw)
To: Junio C Hamano, Taylor Blau; +Cc: Git List, Jeff King
Am 11.09.23 um 21:19 schrieb Junio C Hamano:
> Taylor Blau <me@ttaylorr.com> writes:
>
>> callback, something like:
>>
>> struct option {
>> /* ... */
>> union {
>> void *value;
>> int *value_int;
>> /* etc ... */
>> } u;
>> enum option_type t;
>> };
>>
>> where option_type has some value corresponding to "void *", another for
>> "int *", and so on.
>
> Yup, that does cross my mind, even though I would have used
>
> union {
> void *void_ptr;
> int *int_ptr;
> } value;
>
> or something without a rather meaningless 'u'.
OK, but I neglected to ask what we would get out of throwing different
types into the same bin. It complicates type safety by making it
impossible for the parser to distinguish the used type. This will
become relevant once all int options are converted and value_int can be
made mandatory for them. A named union also requires changing all
users.
It reduces the memory footprint, but only slightly. Saving a few bytes
for objects with less than a hundred instances total doesn't seem worth
the downsides.
René
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
2023-09-18 9:28 ` René Scharfe
@ 2023-09-18 10:10 ` Oswald Buddenhagen
2023-09-19 7:41 ` René Scharfe
2023-09-18 13:33 ` [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE Phillip Wood
1 sibling, 1 reply; 30+ messages in thread
From: Oswald Buddenhagen @ 2023-09-18 10:10 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, Jeff King, Git List
On Mon, Sep 18, 2023 at 11:28:31AM +0200, René Scharfe wrote:
>@@ -2300,12 +2301,12 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar
> "--show-current-patch", arg);
> }
>
>- if (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
>+ if (resume->mode_int == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
>
this illustrates why i don't quite like the approach: the context
determines which variable to use.
my idea would be to introduce a new type OPTION_SET_ENUM which would
also use the callback field. one could even adjust the data type and
elide the callback when c23 mode (or more specifically, the enum size
feature) is detected.
> return error(_("options '%s=%s' and '%s=%s' "
> "cannot be used together"),
> "--show-current-patch", "--show-current-patch", arg, valid_modes[resume->sub_mode]);
>
totally on a tangent: the argument order is bogus here.
and the line wrapping is also funny.
regards
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] parse-options: add int value pointer to struct option
2023-09-18 9:53 ` René Scharfe
@ 2023-09-18 10:28 ` Oswald Buddenhagen
2023-09-18 16:17 ` Junio C Hamano
1 sibling, 0 replies; 30+ messages in thread
From: Oswald Buddenhagen @ 2023-09-18 10:28 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, Taylor Blau, Git List, Jeff King
On Mon, Sep 18, 2023 at 11:53:19AM +0200, René Scharfe wrote:
>Am 11.09.23 um 21:19 schrieb Junio C Hamano:
>> Yup, that does cross my mind, even though I would have used
>>
>> union {
>> void *void_ptr;
>> int *int_ptr;
>> } value;
>>
>> or something without a rather meaningless 'u'.
>
>OK, but I neglected to ask what we would get out of throwing different
>types into the same bin.
>It complicates type safety by making it impossible for the parser to
>distinguish the used type. [...]
>
this is somewhat ironic, given that using a union has some semantic
value by illustrating that the fields are mutually exclusive.
but i'm not sure that the checking is really important here, given that
the initializers are inside centralized macros anyway.
regards
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] parse-options: add int value pointer to struct option
2023-09-11 22:28 ` Oswald Buddenhagen
@ 2023-09-18 11:34 ` Kristoffer Haugsbakk
0 siblings, 0 replies; 30+ messages in thread
From: Kristoffer Haugsbakk @ 2023-09-18 11:34 UTC (permalink / raw)
To: Oswald Buddenhagen
Cc: Taylor Blau, René Scharfe, Git List, Jeff King, Junio C Hamano
On Tue, Sep 12, 2023, at 00:28, Oswald Buddenhagen wrote:
> i'd go the opposite way and make it an anonymous union.
> that would require c11, though. imo not exactly an outrageous
> proposition in 2023, but ...
Moving to C11 would get pushback because not all platforms
that people care about support that compiler.[1]
[1] https://lore.kernel.org/git/004601d8ed6b$13a2f580$3ae8e080$@nexbridge.com/
--
Kristoffer Haugsbakk
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
2023-09-18 9:28 ` René Scharfe
2023-09-18 10:10 ` Oswald Buddenhagen
@ 2023-09-18 13:33 ` Phillip Wood
2023-09-18 17:11 ` Junio C Hamano
2023-09-19 7:47 ` René Scharfe
1 sibling, 2 replies; 30+ messages in thread
From: Phillip Wood @ 2023-09-18 13:33 UTC (permalink / raw)
To: René Scharfe, Junio C Hamano; +Cc: Jeff King, Oswald Buddenhagen, Git List
Hi René
On 18/09/2023 10:28, René Scharfe wrote:
> Here's a version that preserves the enums by using additional int
> variables just for the parsing phase. No tricks. The diff is long, but
> most changes aren't particularly complicated and the resulting code is
> not that ugly. Except for builtin/am.c perhaps, which changes the
> command mode value using a callback as well.
>
> diff --git a/builtin/am.c b/builtin/am.c
> index 202040b62e..00930e2152 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -2270,13 +2270,14 @@ enum resume_type {
>
> struct resume_mode {
> enum resume_type mode;
> + int mode_int;
> enum show_patch_type sub_mode;
> };
>
> static int parse_opt_show_current_patch(const struct option *opt, const char *arg, int unset)
> {
> int *opt_value = opt->value;
> - struct resume_mode *resume = container_of(opt_value, struct resume_mode, mode);
> + struct resume_mode *resume = container_of(opt_value, struct resume_mode, mode_int);
>
> /*
> * Please update $__git_showcurrentpatch in git-completion.bash
> @@ -2300,12 +2301,12 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar
> "--show-current-patch", arg);
> }
>
> - if (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
> + if (resume->mode_int == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
> return error(_("options '%s=%s' and '%s=%s' "
> "cannot be used together"),
> "--show-current-patch", "--show-current-patch", arg, valid_modes[resume->sub_mode]);
>
> - resume->mode = RESUME_SHOW_PATCH;
> + resume->mode_int = RESUME_SHOW_PATCH;
> resume->sub_mode = new_value;
> return 0;
> }
Having "mode" and "mode_int" feels a bit fragile as only "mode_int" is
valid while parsing the options but then we want to use "mode". I wonder
if we could get Oswald's idea of using callbacks working in a reasonably
ergonomic way with a couple of macros. We could add an new
OPTION_SET_ENUM member to "enum parse_opt_type" that would take a setter
function as well as the usual void *value. To set the value it would
pass the value pointer and an integer value to the setter function. We
could change OPT_CMDMODE to use OPTION_SET_ENUM and take the name of the
enum as well as the integer value we want to set for that option. The
name of the enum would be used to generate the name of the setter
callback which would be defined with another macro. The macro to
generate the setter would look like
#define MAKE_CMDMODE_SETTER(name) \
static void parse_cmdmode_ ## name (void * var, int value) {
enum name *p = var;
*p = value;
}
then OPT_CMDMODE would look like
#define OPT_CMDMODE_F(s, l, v, h, n, i, f) { \
.type = OPTION_SET_ENUM, \
.short_name = (s), \
.long_name = (l), \
.value = (v), \
.help = (h), \
.flags = PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG | (f), \
.defval = (i), \
.enum_setter = parse_cmdmode_ ## n,
}
#define OPT_CMDMODE(s, l, v, h, n, i) OPT_CMDMODE_F(s, l, v, h, n, i, 0)
Then in builtin/am.c at the top level we'd add
MAKE_CMDMODE_SETTER(resume_type)
and change the option definitions to look like
OPT_CMDMODE(0, "continue", resume_type, &resume.mode, ...)
Best Wishes
Phillip
> @@ -2316,7 +2317,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)
> int binary = -1;
> int keep_cr = -1;
> int patch_format = PATCH_FORMAT_UNKNOWN;
> - struct resume_mode resume = { .mode = RESUME_FALSE };
> + struct resume_mode resume = { .mode_int = RESUME_FALSE };
> int in_progress;
> int ret = 0;
>
> @@ -2387,27 +2388,27 @@ int cmd_am(int argc, const char **argv, const char *prefix)
> PARSE_OPT_NOARG),
> OPT_STRING(0, "resolvemsg", &state.resolvemsg, NULL,
> N_("override error message when patch failure occurs")),
> - OPT_CMDMODE(0, "continue", &resume.mode,
> + OPT_CMDMODE(0, "continue", &resume.mode_int,
> N_("continue applying patches after resolving a conflict"),
> RESUME_RESOLVED),
> - OPT_CMDMODE('r', "resolved", &resume.mode,
> + OPT_CMDMODE('r', "resolved", &resume.mode_int,
> N_("synonyms for --continue"),
> RESUME_RESOLVED),
> - OPT_CMDMODE(0, "skip", &resume.mode,
> + OPT_CMDMODE(0, "skip", &resume.mode_int,
> N_("skip the current patch"),
> RESUME_SKIP),
> - OPT_CMDMODE(0, "abort", &resume.mode,
> + OPT_CMDMODE(0, "abort", &resume.mode_int,
> N_("restore the original branch and abort the patching operation"),
> RESUME_ABORT),
> - OPT_CMDMODE(0, "quit", &resume.mode,
> + OPT_CMDMODE(0, "quit", &resume.mode_int,
> N_("abort the patching operation but keep HEAD where it is"),
> RESUME_QUIT),
> - { OPTION_CALLBACK, 0, "show-current-patch", &resume.mode,
> + { OPTION_CALLBACK, 0, "show-current-patch", &resume.mode_int,
> "(diff|raw)",
> N_("show the patch being applied"),
> PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
> parse_opt_show_current_patch, RESUME_SHOW_PATCH },
> - OPT_CMDMODE(0, "allow-empty", &resume.mode,
> + OPT_CMDMODE(0, "allow-empty", &resume.mode_int,
> N_("record the empty patch as an empty commit"),
> RESUME_ALLOW_EMPTY),
> OPT_BOOL(0, "committer-date-is-author-date",
> @@ -2439,6 +2440,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)
> am_load(&state);
>
> argc = parse_options(argc, argv, prefix, options, usage, 0);
> + resume.mode = resume.mode_int;
>
> if (binary >= 0)
> fprintf_ln(stderr, _("The -b/--binary option has been a no-op for long time, and\n"
> diff --git a/builtin/help.c b/builtin/help.c
> index dc1fbe2b98..d76f88d544 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -51,6 +51,7 @@ static enum help_action {
> HELP_ACTION_CONFIG_FOR_COMPLETION,
> HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION,
> } cmd_mode;
> +static int cmd_mode_int;
>
> static const char *html_path;
> static int verbose = 1;
> @@ -59,7 +60,7 @@ static int exclude_guides;
> static int show_external_commands = -1;
> static int show_aliases = -1;
> static struct option builtin_help_options[] = {
> - OPT_CMDMODE('a', "all", &cmd_mode, N_("print all available commands"),
> + OPT_CMDMODE('a', "all", &cmd_mode_int, N_("print all available commands"),
> HELP_ACTION_ALL),
> OPT_BOOL(0, "external-commands", &show_external_commands,
> N_("show external commands in --all")),
> @@ -72,19 +73,19 @@ static struct option builtin_help_options[] = {
> HELP_FORMAT_INFO),
> OPT__VERBOSE(&verbose, N_("print command description")),
>
> - OPT_CMDMODE('g', "guides", &cmd_mode, N_("print list of useful guides"),
> + OPT_CMDMODE('g', "guides", &cmd_mode_int, N_("print list of useful guides"),
> HELP_ACTION_GUIDES),
> - OPT_CMDMODE(0, "user-interfaces", &cmd_mode,
> + OPT_CMDMODE(0, "user-interfaces", &cmd_mode_int,
> N_("print list of user-facing repository, command and file interfaces"),
> HELP_ACTION_USER_INTERFACES),
> - OPT_CMDMODE(0, "developer-interfaces", &cmd_mode,
> + OPT_CMDMODE(0, "developer-interfaces", &cmd_mode_int,
> N_("print list of file formats, protocols and other developer interfaces"),
> HELP_ACTION_DEVELOPER_INTERFACES),
> - OPT_CMDMODE('c', "config", &cmd_mode, N_("print all configuration variable names"),
> + OPT_CMDMODE('c', "config", &cmd_mode_int, N_("print all configuration variable names"),
> HELP_ACTION_CONFIG),
> - OPT_CMDMODE_F(0, "config-for-completion", &cmd_mode, "",
> + OPT_CMDMODE_F(0, "config-for-completion", &cmd_mode_int, "",
> HELP_ACTION_CONFIG_FOR_COMPLETION, PARSE_OPT_HIDDEN),
> - OPT_CMDMODE_F(0, "config-sections-for-completion", &cmd_mode, "",
> + OPT_CMDMODE_F(0, "config-sections-for-completion", &cmd_mode_int, "",
> HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION, PARSE_OPT_HIDDEN),
>
> OPT_END(),
> @@ -640,6 +641,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
> argc = parse_options(argc, argv, prefix, builtin_help_options,
> builtin_help_usage, 0);
> parsed_help_format = help_format;
> + cmd_mode = cmd_mode_int;
>
> if (cmd_mode != HELP_ACTION_ALL &&
> (show_external_commands >= 0 ||
> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
> index 209d2dc0d5..c64b38614a 100644
> --- a/builtin/ls-tree.c
> +++ b/builtin/ls-tree.c
> @@ -346,7 +346,8 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
> int i, full_tree = 0;
> int full_name = !prefix || !*prefix;
> read_tree_fn_t fn = NULL;
> - enum ls_tree_cmdmode cmdmode = MODE_DEFAULT;
> + enum ls_tree_cmdmode cmdmode;
> + int cmdmode_int = MODE_DEFAULT;
> int null_termination = 0;
> struct ls_tree_options options = { 0 };
> const struct option ls_tree_options[] = {
> @@ -358,13 +359,13 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
> LS_SHOW_TREES),
> OPT_BOOL('z', NULL, &null_termination,
> N_("terminate entries with NUL byte")),
> - OPT_CMDMODE('l', "long", &cmdmode, N_("include object size"),
> + OPT_CMDMODE('l', "long", &cmdmode_int, N_("include object size"),
> MODE_LONG),
> - OPT_CMDMODE(0, "name-only", &cmdmode, N_("list only filenames"),
> + OPT_CMDMODE(0, "name-only", &cmdmode_int, N_("list only filenames"),
> MODE_NAME_ONLY),
> - OPT_CMDMODE(0, "name-status", &cmdmode, N_("list only filenames"),
> + OPT_CMDMODE(0, "name-status", &cmdmode_int, N_("list only filenames"),
> MODE_NAME_STATUS),
> - OPT_CMDMODE(0, "object-only", &cmdmode, N_("list only objects"),
> + OPT_CMDMODE(0, "object-only", &cmdmode_int, N_("list only objects"),
> MODE_OBJECT_ONLY),
> OPT_BOOL(0, "full-name", &full_name, N_("use full path names")),
> OPT_BOOL(0, "full-tree", &full_tree,
> @@ -384,6 +385,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
> argc = parse_options(argc, argv, prefix, ls_tree_options,
> ls_tree_usage, 0);
> options.null_termination = null_termination;
> + cmdmode = cmdmode_int;
>
> if (full_tree)
> prefix = NULL;
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 50cb85751f..6dbe57f0ac 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1061,6 +1061,7 @@ static int check_exec_cmd(const char *cmd)
> int cmd_rebase(int argc, const char **argv, const char *prefix)
> {
> struct rebase_options options = REBASE_OPTIONS_INIT;
> + int action;
> const char *branch_name;
> int ret, flags, total_argc, in_progress = 0;
> int keep_base = 0;
> @@ -1116,18 +1117,18 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> OPT_BIT(0, "no-ff", &options.flags,
> N_("cherry-pick all commits, even if unchanged"),
> REBASE_FORCE),
> - OPT_CMDMODE(0, "continue", &options.action, N_("continue"),
> + OPT_CMDMODE(0, "continue", &action, N_("continue"),
> ACTION_CONTINUE),
> - OPT_CMDMODE(0, "skip", &options.action,
> + OPT_CMDMODE(0, "skip", &action,
> N_("skip current patch and continue"), ACTION_SKIP),
> - OPT_CMDMODE(0, "abort", &options.action,
> + OPT_CMDMODE(0, "abort", &action,
> N_("abort and check out the original branch"),
> ACTION_ABORT),
> - OPT_CMDMODE(0, "quit", &options.action,
> + OPT_CMDMODE(0, "quit", &action,
> N_("abort but keep HEAD where it is"), ACTION_QUIT),
> - OPT_CMDMODE(0, "edit-todo", &options.action, N_("edit the todo list "
> + OPT_CMDMODE(0, "edit-todo", &action, N_("edit the todo list "
> "during an interactive rebase"), ACTION_EDIT_TODO),
> - OPT_CMDMODE(0, "show-current-patch", &options.action,
> + OPT_CMDMODE(0, "show-current-patch", &action,
> N_("show the patch file being applied or merged"),
> ACTION_SHOW_CURRENT_PATCH),
> OPT_CALLBACK_F(0, "apply", &options, NULL,
> @@ -1233,10 +1234,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> if (options.type != REBASE_UNSPECIFIED)
> in_progress = 1;
>
> + action = options.action;
> total_argc = argc;
> argc = parse_options(argc, argv, prefix,
> builtin_rebase_options,
> builtin_rebase_usage, 0);
> + options.action = action;
>
> if (preserve_merges_selected)
> die(_("--preserve-merges was replaced by --rebase-merges\n"
> diff --git a/builtin/replace.c b/builtin/replace.c
> index da59600ad2..205c337ad3 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -554,12 +554,13 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
> MODE_CONVERT_GRAFT_FILE,
> MODE_REPLACE
> } cmdmode = MODE_UNSPECIFIED;
> + int cmdmode_int = cmdmode;
> struct option options[] = {
> - OPT_CMDMODE('l', "list", &cmdmode, N_("list replace refs"), MODE_LIST),
> - OPT_CMDMODE('d', "delete", &cmdmode, N_("delete replace refs"), MODE_DELETE),
> - OPT_CMDMODE('e', "edit", &cmdmode, N_("edit existing object"), MODE_EDIT),
> - OPT_CMDMODE('g', "graft", &cmdmode, N_("change a commit's parents"), MODE_GRAFT),
> - OPT_CMDMODE(0, "convert-graft-file", &cmdmode, N_("convert existing graft file"), MODE_CONVERT_GRAFT_FILE),
> + OPT_CMDMODE('l', "list", &cmdmode_int, N_("list replace refs"), MODE_LIST),
> + OPT_CMDMODE('d', "delete", &cmdmode_int, N_("delete replace refs"), MODE_DELETE),
> + OPT_CMDMODE('e', "edit", &cmdmode_int, N_("edit existing object"), MODE_EDIT),
> + OPT_CMDMODE('g', "graft", &cmdmode_int, N_("change a commit's parents"), MODE_GRAFT),
> + OPT_CMDMODE(0, "convert-graft-file", &cmdmode_int, N_("convert existing graft file"), MODE_CONVERT_GRAFT_FILE),
> OPT_BOOL_F('f', "force", &force, N_("replace the ref if it exists"),
> PARSE_OPT_NOCOMPLETE),
> OPT_BOOL(0, "raw", &raw, N_("do not pretty-print contents for --edit")),
> @@ -572,6 +573,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
>
> argc = parse_options(argc, argv, prefix, options, git_replace_usage, 0);
>
> + cmdmode = cmdmode_int;
> if (!cmdmode)
> cmdmode = argc ? MODE_REPLACE : MODE_LIST;
>
> diff --git a/builtin/stripspace.c b/builtin/stripspace.c
> index 7b700a9fb1..e8efa0e7ac 100644
> --- a/builtin/stripspace.c
> +++ b/builtin/stripspace.c
> @@ -33,13 +33,14 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
> {
> struct strbuf buf = STRBUF_INIT;
> enum stripspace_mode mode = STRIP_DEFAULT;
> + int mode_int = mode;
> int nongit;
>
> const struct option options[] = {
> - OPT_CMDMODE('s', "strip-comments", &mode,
> + OPT_CMDMODE('s', "strip-comments", &mode_int,
> N_("skip and remove all lines starting with comment character"),
> STRIP_COMMENTS),
> - OPT_CMDMODE('c', "comment-lines", &mode,
> + OPT_CMDMODE('c', "comment-lines", &mode_int,
> N_("prepend comment character and space to each line"),
> COMMENT_LINES),
> OPT_END()
> @@ -49,6 +50,7 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
> if (argc)
> usage_with_options(stripspace_usage, options);
>
> + mode = mode_int;
> if (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {
> setup_git_directory_gently(&nongit);
> git_config(git_default_config, NULL);
> diff --git a/parse-options.h b/parse-options.h
> index 5e7475bd2d..349c3fca04 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -262,7 +262,7 @@ struct option {
> .type = OPTION_SET_INT, \
> .short_name = (s), \
> .long_name = (l), \
> - .value = (v), \
> + .value_int = (v), \
> .help = (h), \
> .flags = PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG | (f), \
> .defval = (i), \
> --
> 2.42.0
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] parse-options: add int value pointer to struct option
2023-09-18 9:53 ` René Scharfe
2023-09-18 10:28 ` Oswald Buddenhagen
@ 2023-09-18 16:17 ` Junio C Hamano
2023-09-20 11:34 ` René Scharfe
1 sibling, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2023-09-18 16:17 UTC (permalink / raw)
To: René Scharfe; +Cc: Taylor Blau, Git List, Jeff King
René Scharfe <l.s.r@web.de> writes:
> It reduces the memory footprint, but only slightly. Saving a few bytes
> for objects with less than a hundred instances total doesn't seem worth
> the downsides.
It makes it impossible to use the both at the same time, which is a
bigger (than reduced memory) advantage. Otherwise, we would be
tempted to consider that having "void *value" and "int value_int"
next to each other and allow them to coexist may be a good solution
for a narrow corner case (please see at the end of the message you
are responding to).
As you said, use of union has its downsides that may contradict the
objective of the larger picture this topic draws.
Thanks.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
2023-09-18 13:33 ` [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE Phillip Wood
@ 2023-09-18 17:11 ` Junio C Hamano
2023-09-18 19:48 ` Phillip Wood
2023-09-19 7:47 ` René Scharfe
1 sibling, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2023-09-18 17:11 UTC (permalink / raw)
To: Phillip Wood; +Cc: René Scharfe, Jeff King, Oswald Buddenhagen, Git List
Phillip Wood <phillip.wood123@gmail.com> writes:
>> - resume->mode = RESUME_SHOW_PATCH;
>> + resume->mode_int = RESUME_SHOW_PATCH;
>> resume->sub_mode = new_value;
>> return 0;
>> }
>
> Having "mode" and "mode_int" feels a bit fragile as only "mode_int" is
> valid while parsing the options but then we want to use "mode". I
> wonder if we could get Oswald's idea of using callbacks working in a
> reasonably ergonomic way with a couple of macros. We could add an new
> OPTION_SET_ENUM member to "enum parse_opt_type" that would take a
> setter function as well as the usual void *value. To set the value it
> would pass the value pointer and an integer value to the setter
> function. We could change OPT_CMDMODE to use OPTION_SET_ENUM and take
> the name of the enum as well as the integer value we want to set for
> that option. The name of the enum would be used to generate the name
> of the setter callback which would be defined with another macro. The
> macro to generate the setter would look like
>
> #define MAKE_CMDMODE_SETTER(name) \
> static void parse_cmdmode_ ## name (void * var, int value) {
> enum name *p = var;
> *p = value;
> }
Ah, OK. So that's how you defeat "how the size and alignment of an
enum mixes well with int is not known and depends on particular enum
type". It is a tad sad that this relies on "void *", which means
that the caller of parse_cmdmode_resume_type cannot be forced by the
compilers to pass "enum resume_type *" to the function, though. And
that is probably inevitable with the design as .enum_setter needs to
be of a single type, and the member in the "struct option" that
points at the destination variable must be "void *" as it has to
be capable of pointing at various different enum types.
> ...
> Then in builtin/am.c at the top level we'd add
>
> MAKE_CMDMODE_SETTER(resume_type)
>
> and change the option definitions to look like
>
> OPT_CMDMODE(0, "continue", resume_type, &resume.mode, ...)
Yup, that is ergonomic and corrects "The shape of a particular enum
may not match 'int'" issue nicely. I do not know how severe the
problem is that it is not quite type safe that we cannot enforce
resume_type is the same as typeof(resume.mode) here, though.
Thanks.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
2023-09-18 17:11 ` Junio C Hamano
@ 2023-09-18 19:48 ` Phillip Wood
0 siblings, 0 replies; 30+ messages in thread
From: Phillip Wood @ 2023-09-18 19:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: René Scharfe, Jeff King, Oswald Buddenhagen, Git List
On 18/09/2023 18:11, Junio C Hamano wrote:
>> Then in builtin/am.c at the top level we'd add
>>
>> MAKE_CMDMODE_SETTER(resume_type)
>>
>> and change the option definitions to look like
>>
>> OPT_CMDMODE(0, "continue", resume_type, &resume.mode, ...)
>
> Yup, that is ergonomic and corrects "The shape of a particular enum
> may not match 'int'" issue nicely. I do not know how severe the
> problem is that it is not quite type safe that we cannot enforce
> resume_type is the same as typeof(resume.mode) here, though.
We could use gcc's __builtin_types_compatible_p() if we're prepared to
have two definitions of OPT_CMDMODE_F
#if defined(__GNUC__)
#define OPT_CMDMODE_F(s, l, n, v, h, i, f) { \
...
.defval (i) + \
BUILD_ASSERT_OR_ZERO(__builtin_types_compatible_p(enum n,
__typeof__(v))), \
}
#else
#define OPT_CMDMODE_F(s, l, n, v, h, i, f) { \
...
.defval (i), \
}
#endif
Best Wishes
Phillip
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
2023-09-18 10:10 ` Oswald Buddenhagen
@ 2023-09-19 7:41 ` René Scharfe
2023-09-21 11:07 ` [PATCH] am: fix error message in parse_opt_show_current_patch() Oswald Buddenhagen
0 siblings, 1 reply; 30+ messages in thread
From: René Scharfe @ 2023-09-19 7:41 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: Junio C Hamano, Jeff King, Git List
Am 18.09.23 um 12:10 schrieb Oswald Buddenhagen:
> On Mon, Sep 18, 2023 at 11:28:31AM +0200, René Scharfe wrote:
>>
>> return error(_("options '%s=%s' and '%s=%s' "
>> "cannot be used together"),
>
>> "--show-current-patch", "--show-current-patch", arg, valid_modes[resume->sub_mode]);
>>
> totally on a tangent: the argument order is bogus here.
> and the line wrapping is also funny.
Hah, good catch! Care to send a patch?
René
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
2023-09-18 13:33 ` [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE Phillip Wood
2023-09-18 17:11 ` Junio C Hamano
@ 2023-09-19 7:47 ` René Scharfe
1 sibling, 0 replies; 30+ messages in thread
From: René Scharfe @ 2023-09-19 7:47 UTC (permalink / raw)
To: phillip.wood, Junio C Hamano; +Cc: Jeff King, Oswald Buddenhagen, Git List
Am 18.09.23 um 15:33 schrieb Phillip Wood:
> Hi René
>
> On 18/09/2023 10:28, René Scharfe wrote:
>> Here's a version that preserves the enums by using additional int
>> variables just for the parsing phase. No tricks. The diff is long, but
>> most changes aren't particularly complicated and the resulting code is
>> not that ugly. Except for builtin/am.c perhaps, which changes the
>> command mode value using a callback as well.
>>
>
>> diff --git a/builtin/am.c b/builtin/am.c
>> index 202040b62e..00930e2152 100644
>> --- a/builtin/am.c
>> +++ b/builtin/am.c
>> @@ -2270,13 +2270,14 @@ enum resume_type {
>>
>> struct resume_mode {
>> enum resume_type mode;
>> + int mode_int;
>> enum show_patch_type sub_mode;
>> };
>>
>> static int parse_opt_show_current_patch(const struct option *opt, const char *arg, int unset)
>> {
>> int *opt_value = opt->value;
>> - struct resume_mode *resume = container_of(opt_value, struct resume_mode, mode);
>> + struct resume_mode *resume = container_of(opt_value, struct resume_mode, mode_int);
>>
>> /*
>> * Please update $__git_showcurrentpatch in git-completion.bash
>> @@ -2300,12 +2301,12 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar
>> "--show-current-patch", arg);
>> }
>>
>> - if (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
>> + if (resume->mode_int == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
>> return error(_("options '%s=%s' and '%s=%s' "
>> "cannot be used together"),
>> "--show-current-patch", "--show-current-patch", arg, valid_modes[resume->sub_mode]);
>>
>> - resume->mode = RESUME_SHOW_PATCH;
>> + resume->mode_int = RESUME_SHOW_PATCH;
>> resume->sub_mode = new_value;
>> return 0;
>> }
>
> Having "mode" and "mode_int" feels a bit fragile as only "mode_int"
> is valid while parsing the options but then we want to use "mode".
True. It feels a bit worse here than for the others because there the
variable is on the stack and here it's in a struct that is passed
around.
> I wonder if we could get Oswald's idea of using callbacks working in
> a reasonably ergonomic way with a couple of macros. We could add an
> new OPTION_SET_ENUM member to "enum parse_opt_type" that would take
> a setter function as well as the usual void *value. To set the value
> it would pass the value pointer and an integer value to the setter
> function. We could change OPT_CMDMODE to use OPTION_SET_ENUM and
> take the name of the enum as well as the integer value we want to set
> for that option. The name of the enum would be used to generate the
> name of the setter callback which would be defined with another
> macro. The macro to generate the setter would look like
>
> #define MAKE_CMDMODE_SETTER(name) \
> static void parse_cmdmode_ ## name (void * var, int value) {
> enum name *p = var;
> *p = value;
> }
>
> then OPT_CMDMODE would look like
>
> #define OPT_CMDMODE_F(s, l, v, h, n, i, f) { \
> .type = OPTION_SET_ENUM, \
> .short_name = (s), \
> .long_name = (l), \
> .value = (v), \
> .help = (h), \
> .flags = PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG | (f), \
> .defval = (i), \
> .enum_setter = parse_cmdmode_ ## n,
> }
> #define OPT_CMDMODE(s, l, v, h, n, i) OPT_CMDMODE_F(s, l, v, h, n, i, 0)
>
>
> Then in builtin/am.c at the top level we'd add
>
> MAKE_CMDMODE_SETTER(resume_type)
>
> and change the option definitions to look like
>
> OPT_CMDMODE(0, "continue", resume_type, &resume.mode, ...)
About half of the OPT_CMDMODE users use int, not enum. They'd have to
be converted. On the flipside the direct and indirect uses of
OPT_SET_INT_F with enums could be converted to an OPT_SET_ENUM_F based
on the above to avoid their use of incompatible pointers as well (e.g.
OPT_IPVERSION).
For uses of OPT_BIT and OPT_NEGBIT with enums a getter would be needed
as well, though (e.g. git ls-tree -d/-r/-t).
At this point I wonder if the existing callback mechanism would suffice.
Which brings me full circle to the topic of typed callbacks. Perhaps I
should introduce them first and come back to solving the int/enum issue
at the end of that journey.
René
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
2023-09-09 21:14 ` [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE René Scharfe
2023-09-10 10:18 ` Oswald Buddenhagen
2023-09-11 19:12 ` Junio C Hamano
@ 2023-09-19 9:40 ` Oswald Buddenhagen
2023-09-20 8:18 ` René Scharfe
2 siblings, 1 reply; 30+ messages in thread
From: Oswald Buddenhagen @ 2023-09-19 9:40 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Jeff King, Junio C Hamano
On Sat, Sep 09, 2023 at 11:14:20PM +0200, René Scharfe wrote:
>Some uses of OPT_CMDMODE provide a pointer to an enum. It is
>dereferenced as an int pointer in parse-options.c::get_value(). These
>two types are incompatible, though
>
s/are/may be/ - c.f. https://en.cppreference.com/w/c/language/enum
>-- the storage size of an enum can vary between platforms.
>
here's a completely different perspective:
this is merely a theoretical problem, right? gcc for example won't
actually use non-ints unless -fshort-enums is supplied. so how about
simply adding a (configure) test to ensure that there is actually no
problem, and calling it a day?
regards
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
2023-09-19 9:40 ` Oswald Buddenhagen
@ 2023-09-20 8:18 ` René Scharfe
2023-09-21 10:40 ` Oswald Buddenhagen
0 siblings, 1 reply; 30+ messages in thread
From: René Scharfe @ 2023-09-20 8:18 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: Git List, Jeff King, Junio C Hamano
Am 19.09.23 um 11:40 schrieb Oswald Buddenhagen:
> On Sat, Sep 09, 2023 at 11:14:20PM +0200, René Scharfe wrote:
>> Some uses of OPT_CMDMODE provide a pointer to an enum. It is
>> dereferenced as an int pointer in parse-options.c::get_value(). These
>> two types are incompatible, though
>>
> s/are/may be/ - c.f. https://en.cppreference.com/w/c/language/enum
You're right. Citing the relevant part: "Each enumerated type [...] is
compatible with one of: char, a signed integer type, or an unsigned
integer type [...]. It is implementation-defined which type is
compatible with any given enumerated type [...]." So there's a chance
that the underlying type would be compatible by accident.
When we try a few combinations (https://godbolt.org/z/KvKcndY4Y),
Clang warns about incompatible pointers if we use a pointer to an enum
with only positive values as int pointer and about different signs if
use a pointer to an enum with negative values as in unsigned int
pointer and accepts the rest. GCC accepts the same cases, but all its
warnings are about incompatible pointers. This seems to be dependent
on the optimization level, though. MSVC warns about all combinations.
>> -- the storage size of an enum can vary between platforms.
>>
> here's a completely different perspective:
> this is merely a theoretical problem, right? gcc for example won't
> actually use non-ints unless -fshort-enums is supplied. so how about
> simply adding a (configure) test to ensure that there is actually no
> problem, and calling it a day?
That would be an easy, but complex solution. If the check is done
using -Wincompatible-pointer-types or equivalent then MSVC is out. If
we base it on type size then we're making assumptions that I find hard
to justify. Using the same type at both ends of the void and avoiding
compiler warnings that would have been issued if we'd cut out the
middle part is simpler overall.
René
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] parse-options: add int value pointer to struct option
2023-09-18 16:17 ` Junio C Hamano
@ 2023-09-20 11:34 ` René Scharfe
0 siblings, 0 replies; 30+ messages in thread
From: René Scharfe @ 2023-09-20 11:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Taylor Blau, Git List, Jeff King
Am 18.09.23 um 18:17 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> It reduces the memory footprint, but only slightly. Saving a few bytes
>> for objects with less than a hundred instances total doesn't seem worth
>> the downsides.
>
> It makes it impossible to use the both at the same time, which is a
> bigger (than reduced memory) advantage. Otherwise, we would be
> tempted to consider that having "void *value" and "int value_int"
> next to each other and allow them to coexist may be a good solution
> for a narrow corner case (please see at the end of the message you
> are responding to).
Good point. The patch adds a check to parse_options_check() to prevent
double use, which adds some runtime overhead and doesn't fully remove
the temptation.
René
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
2023-09-20 8:18 ` René Scharfe
@ 2023-09-21 10:40 ` Oswald Buddenhagen
0 siblings, 0 replies; 30+ messages in thread
From: Oswald Buddenhagen @ 2023-09-21 10:40 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Jeff King, Junio C Hamano
On Wed, Sep 20, 2023 at 10:18:10AM +0200, René Scharfe wrote:
> MSVC warns about all combinations.
>
yes, though that's not a problem: after we established that the
underlying type is int, we can just have a cast in the initializer
macro.
>> so how about simply adding a (configure) test to ensure that there is
>> actually no problem, and calling it a day?
> If we base it on type size then we're making assumptions that I find
> hard to justify.
>
the only one i can think of is signedness. i think this can be safely
ignored as long as we use only small positive integers.
regards
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH] am: fix error message in parse_opt_show_current_patch()
2023-09-19 7:41 ` René Scharfe
@ 2023-09-21 11:07 ` Oswald Buddenhagen
2023-09-21 19:09 ` Junio C Hamano
0 siblings, 1 reply; 30+ messages in thread
From: Oswald Buddenhagen @ 2023-09-21 11:07 UTC (permalink / raw)
To: git
Cc: Jean-Noël Avila, Johannes Sixt, René Scharfe, Junio C Hamano
The argument order was incorrect. This was introduced by 246cac8505
(i18n: turn even more messages into "cannot be used together" ones,
2022-01-05).
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
fwiw, this is currently the only message that actually uses the %s=%s
format, so as of now, factoring out the argument names has only
theoretical value.
Cc: Jean-Noël Avila <jn.avila@free.fr>
Cc: Johannes Sixt <j6t@kdbg.org>
Cc: René Scharfe <l.s.r@web.de>
Cc: Junio C Hamano <gitster@pobox.com>
---
builtin/am.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/builtin/am.c b/builtin/am.c
index 202040b62e..6655059a57 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2303,7 +2303,8 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar
if (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
return error(_("options '%s=%s' and '%s=%s' "
"cannot be used together"),
- "--show-current-patch", "--show-current-patch", arg, valid_modes[resume->sub_mode]);
+ "--show-current-patch", arg,
+ "--show-current-patch", valid_modes[resume->sub_mode]);
resume->mode = RESUME_SHOW_PATCH;
resume->sub_mode = new_value;
--
2.42.0.419.g70bf8a5751
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH] am: fix error message in parse_opt_show_current_patch()
2023-09-21 11:07 ` [PATCH] am: fix error message in parse_opt_show_current_patch() Oswald Buddenhagen
@ 2023-09-21 19:09 ` Junio C Hamano
2023-09-21 19:28 ` Oswald Buddenhagen
0 siblings, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2023-09-21 19:09 UTC (permalink / raw)
To: Oswald Buddenhagen
Cc: git, Jean-Noël Avila, Johannes Sixt, René Scharfe
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> The argument order was incorrect. This was introduced by 246cac8505
> (i18n: turn even more messages into "cannot be used together" ones,
> 2022-01-05).
>
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
>
> ---
> fwiw, this is currently the only message that actually uses the %s=%s
> format, so as of now, factoring out the argument names has only
> theoretical value.
I am not sure I follow, if you mean that the programmer needs to
pass "--show-current-patch" only once if we used something like
"%1$s=%2s and %1$s=%3s", I agree that it probably has little value.
The patch looks good. It seems that we are seeing a crack in test
coverage, by the way?
Will queue. Thanks.
> diff --git a/builtin/am.c b/builtin/am.c
> index 202040b62e..6655059a57 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -2303,7 +2303,8 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar
> if (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
> return error(_("options '%s=%s' and '%s=%s' "
> "cannot be used together"),
> - "--show-current-patch", "--show-current-patch", arg, valid_modes[resume->sub_mode]);
> + "--show-current-patch", arg,
> + "--show-current-patch", valid_modes[resume->sub_mode]);
>
> resume->mode = RESUME_SHOW_PATCH;
> resume->sub_mode = new_value;
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] am: fix error message in parse_opt_show_current_patch()
2023-09-21 19:09 ` Junio C Hamano
@ 2023-09-21 19:28 ` Oswald Buddenhagen
0 siblings, 0 replies; 30+ messages in thread
From: Oswald Buddenhagen @ 2023-09-21 19:28 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jean-Noël Avila, Johannes Sixt, René Scharfe
On Thu, Sep 21, 2023 at 12:09:10PM -0700, Junio C Hamano wrote:
>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>> fwiw, this is currently the only message that actually uses the %s=%s
>> format, so as of now, factoring out the argument names has only
>> theoretical value.
>
>I am not sure I follow, if you mean that the programmer needs to
>pass "--show-current-patch" only once if we used something like
>"%1$s=%2s and %1$s=%3s", I agree that it probably has little value.
>
no, i mean that that the usual pattern is just "options '%s' and '%s'
cannot be used together". this format string is indeed used many times,
so it makes sense to factor out the option names to avoid duplication of
translatable strings. not so here. but this particular case is still a
lot less specialized than many of the other strings replaced by the
referenced patch, and it's at least plausible that further uses would be
added at some point, so i left it as-is.
i thought about the duplication of the option string as well, but
compilers should merge the string constants, so that part is indeed
de-duplicated. the cost of the extra pointer push could be avoided by
use of the %1$s syntax, but afaict that's unprecedented in git, and i
kind of expect that some printf implementation would throw up from it.
also, it might reduce the chance of the format being used in another
place, but who knows.
regards
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2023-09-21 19:47 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-09 21:10 [PATCH 1/2] parse-options: add int value pointer to struct option René Scharfe
2023-09-09 21:14 ` [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE René Scharfe
2023-09-10 10:18 ` Oswald Buddenhagen
2023-09-11 20:11 ` René Scharfe
2023-09-12 8:40 ` Jeff King
2023-09-16 17:45 ` Junio C Hamano
2023-09-18 9:28 ` René Scharfe
2023-09-18 10:10 ` Oswald Buddenhagen
2023-09-19 7:41 ` René Scharfe
2023-09-21 11:07 ` [PATCH] am: fix error message in parse_opt_show_current_patch() Oswald Buddenhagen
2023-09-21 19:09 ` Junio C Hamano
2023-09-21 19:28 ` Oswald Buddenhagen
2023-09-18 13:33 ` [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE Phillip Wood
2023-09-18 17:11 ` Junio C Hamano
2023-09-18 19:48 ` Phillip Wood
2023-09-19 7:47 ` René Scharfe
2023-09-11 19:12 ` Junio C Hamano
2023-09-11 20:11 ` René Scharfe
2023-09-19 9:40 ` Oswald Buddenhagen
2023-09-20 8:18 ` René Scharfe
2023-09-21 10:40 ` Oswald Buddenhagen
2023-09-10 18:40 ` [PATCH 1/2] parse-options: add int value pointer to struct option Taylor Blau
2023-09-11 19:19 ` Junio C Hamano
2023-09-11 22:28 ` Oswald Buddenhagen
2023-09-18 11:34 ` Kristoffer Haugsbakk
2023-09-18 9:53 ` René Scharfe
2023-09-18 10:28 ` Oswald Buddenhagen
2023-09-18 16:17 ` Junio C Hamano
2023-09-20 11:34 ` René Scharfe
2023-09-11 20:12 ` René Scharfe
Code repositories for project(s) associated with this public inbox
https://80x24.org/pub/scm/git/git.git/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).