Re: [PATCH 09/13] parse-options API: don't restrict OPT_SUBCOMMAND() to one *_fn type

> Am 05.11.22 um 14:52 schrieb Ævar Arnfjörð Bjarmason:
>>
>> I think that's an "unportable" extension covered in "J.5 Common
>> extensions", specifically "J.5.7 Function pointer casts":
>>
>> 	A pointer to an object or to void may be cast to a pointer to a
>> 	function, allowing data to be invoked as a function
>>
>> Thus, since the standard already establishes that valid "void *" and
>> "intptr_t" pointers can be cast'd back & forth, the J.5.7 bridges the
>> gap between the two saying a function pointer can be converted to
>> either.
>>
>> Now, I may be missing something here, but I was under the impression
>> that "intptr_t" wasn't special in any way here, and that any casting of
>> a function pointer to either it or a "void *" was what was made portable
>> by "J.5.7".
>
> Do you mean "possible" or "workable" instead of "portable" here?  As you
> write above, J.5.7 is an extension, not (fully) portable.

>> Anyway, like ssize_t and a few other things this is extended upon and
>> made standard by POSIX. I.e. we're basically talking about whether this
>> passes:
>>
>> 	assert(sizeof(void (*)(void)) == sizeof(void*))
>>
>> And per POSIX
>> (https://pubs.opengroup.org/onlinepubs/9699919799/functions/dlsym.html):
>>
>> 	Note that conversion from a void * pointer to a function pointer
>> 	as in:
>>
>> 		fptr = (int (*)(int))dlsym(handle, "my_function");
>>
>> 	is not defined by the ISO C standard. This standard requires
>> 	this conversion to work correctly on conforming implementations.
>
> Conversion from object pointer to function pointer can still work if
> function pointers are wider.
>
>> So I think aside from other concerns this should be safe to use, as
>> real-world data backing that up we've had a intptr_t converted to a
>> function pointer since v2.35.0: 5cb28270a1f (pack-objects: lazily set up
>> "struct rev_info", don't leak, 2022-03-28).
>
> That may not have reached unusual architectures, yet.  Let's replace
> that cast with something boring before someone gets hurt.  Something
> like this?
>
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 573d0b20b7..9e6f1530c6 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -4154,14 +4154,15 @@ struct po_filter_data {
>  	struct rev_info revs;
>  };
>
> -static struct list_objects_filter_options *po_filter_revs_init(void *value)
> +static int list_objects_filter_cb(const struct option *opt,
> +				  const char *arg, int unset)
>  {
> -	struct po_filter_data *data = value;
> +	struct po_filter_data *data = opt->value;
>
>  	repo_init_revisions(the_repository, &data->revs, NULL);
>  	data->have_revs = 1;
>
> -	return &data->revs.filter;
> +	return opt_parse_list_objects_filter(&data->revs.filter, arg, unset);
>  }
>
>  int cmd_pack_objects(int argc, const char **argv, const char *prefix)
> @@ -4265,7 +4266,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
>  			      &write_bitmap_index,
>  			      N_("write a bitmap index if possible"),
>  			      WRITE_BITMAP_QUIET, PARSE_OPT_HIDDEN),
> -		OPT_PARSE_LIST_OBJECTS_FILTER_INIT(&pfd, po_filter_revs_init),
> +		OPT_PARSE_LIST_OBJECTS_FILTER_F(&pfd, list_objects_filter_cb),
>  		OPT_CALLBACK_F(0, "missing", NULL, N_("action"),
>  		  N_("handling for missing objects"), PARSE_OPT_NONEG,
>  		  option_parse_missing_action),
> diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
> index 5339660238..2e560c2fdb 100644
> --- a/list-objects-filter-options.c
> +++ b/list-objects-filter-options.c
> @@ -286,15 +286,9 @@ void parse_list_objects_filter(
>  		die("%s", errbuf.buf);
>  }
>
> -int opt_parse_list_objects_filter(const struct option *opt,
> +int opt_parse_list_objects_filter(struct list_objects_filter_options *filter_options,
>  				  const char *arg, int unset)
>  {
> -	struct list_objects_filter_options *filter_options = opt->value;
> -	opt_lof_init init = (opt_lof_init)opt->defval;
> -
> -	if (init)
> -		filter_options = init(opt->value);
> -
>  	if (unset || !arg)
>  		list_objects_filter_set_no_filter(filter_options);
>  	else
> @@ -302,6 +296,12 @@ int opt_parse_list_objects_filter(const struct option *opt,
>  	return 0;
>  }
>
> +int opt_parse_list_objects_filter_cb(const struct option *opt,
> +				     const char *arg, int unset)
> +{
> +	return opt_parse_list_objects_filter(opt->value, arg, unset);
> +}
> +
>  const char *list_objects_filter_spec(struct list_objects_filter_options *filter)
>  {
>  	if (!filter->filter_spec.len)
> diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h
> index 7eeadab2dd..fc6b4da06d 100644
> --- a/list-objects-filter-options.h
> +++ b/list-objects-filter-options.h
> @@ -107,31 +107,26 @@ void parse_list_objects_filter(
>  	struct list_objects_filter_options *filter_options,
>  	const char *arg);
>
> +int opt_parse_list_objects_filter(struct list_objects_filter_options *filter_options,
> +				  const char *arg, int unset);
> +
>  /**
>   * The opt->value to opt_parse_list_objects_filter() is either a
>   * "struct list_objects_filter_option *" when using
>   * OPT_PARSE_LIST_OBJECTS_FILTER().
>   *
> - * Or, if using no "struct option" field is used by the callback,
> - * except the "defval" which is expected to be an "opt_lof_init"
> - * function, which is called with the "opt->value" and must return a
> - * pointer to the ""struct list_objects_filter_option *" to be used.
> - *
> - * The OPT_PARSE_LIST_OBJECTS_FILTER_INIT() can be used e.g. the
> - * "struct list_objects_filter_option" is embedded in a "struct
> - * rev_info", which the "defval" could be tasked with lazily
> - * initializing. See cmd_pack_objects() for an example.
> + * Or, OPT_PARSE_LIST_OBJECTS_FILTER_F() can be used to specify a
> + * custom callback function that may expect a different type.
>   */
> -int opt_parse_list_objects_filter(const struct option *opt,
> -				  const char *arg, int unset);
> +int opt_parse_list_objects_filter_cb(const struct option *opt,
> +				     const char *arg, int unset);
>  typedef struct list_objects_filter_options *(*opt_lof_init)(void *);
> -#define OPT_PARSE_LIST_OBJECTS_FILTER_INIT(fo, init) \
> +#define OPT_PARSE_LIST_OBJECTS_FILTER_F(fo, fn) \
>  	{ OPTION_CALLBACK, 0, "filter", (fo), N_("args"), \
> -	  N_("object filtering"), 0, opt_parse_list_objects_filter, \
> -	  (intptr_t)(init) }
> +	  N_("object filtering"), 0, (fn) }
>
>  #define OPT_PARSE_LIST_OBJECTS_FILTER(fo) \
> -	OPT_PARSE_LIST_OBJECTS_FILTER_INIT((fo), NULL)
> +	OPT_PARSE_LIST_OBJECTS_FILTER_F((fo), opt_parse_list_objects_filter_cb)
>
>  /*
>   * Translates abbreviated numbers in the filter's filter_spec into their