Git Mailing List Archive mirror
 help / color / mirror / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: "René Scharfe" <l.s.r@web.de>
Cc: git@vger.kernel.org, "Junio C Hamano" <gitster@pobox.com>,
	"Jeff King" <peff@peff.net>, "Taylor Blau" <me@ttaylorr.com>,
	"Đoàn Trần Công Danh" <congdanhqx@gmail.com>,
	"SZEDER Gábor" <szeder.dev@gmail.com>,
	"Lukáš Doktor" <ldoktor@redhat.com>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>
Subject: Re: [PATCH 09/13] parse-options API: don't restrict OPT_SUBCOMMAND() to one *_fn type
Date: Sat, 05 Nov 2022 23:33:32 +0100	[thread overview]
Message-ID: <221105.86o7tlvxh0.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <25776063-a672-fc65-bed3-1bc8536ab8b3@web.de>


On Sat, Nov 05 2022, René Scharfe wrote:

> 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.

I think my just-sent in the side-thread should clarify this.

>> 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

I think "just leave it, and see if anyone complains".

If you look over config.mak.uname you can see what we're likely to be
ported to (and some of that's probably dead). The list of potential
targets that:

 1) We know of ports to, or people would plausibly port git to
 2) Are updated so slow that they're on a release that's getting close
    to a year old.

Are small, and it's usually easy to look up their memory model etc. are
you concerned about any specific one?

I think if you're worried enough about it to push for the diff above:
Can we just hide it behind an "#ifdef", then if we find that nobody's
using it, we can consider it safe to use.

I don't think there's any great benefit to the extension in that
specific case, but there might be in the future (e.g. this topic would
be one small user), so since we already have an unintentional test
ballon, why not see if we can keep it safely?

  reply	other threads:[~2022-11-05 22:38 UTC|newest]

Thread overview: 106+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-04  6:31 "git bisect run" strips "--log" from the list of arguments Lukáš Doktor
2022-11-04  9:45 ` Jeff King
2022-11-04 11:10   ` Đoàn Trần Công Danh
2022-11-04 12:51     ` Jeff King
2022-11-04 11:36   ` Ævar Arnfjörð Bjarmason
2022-11-04 12:45     ` Jeff King
2022-11-04 13:07       ` Ævar Arnfjörð Bjarmason
2022-11-04 12:37   ` SZEDER Gábor
2022-11-04 12:44     ` Jeff King
2022-11-04 11:40 ` [PATCH 0/3] Convert git-bisect--helper to OPT_SUBCOMMAND Đoàn Trần Công Danh
2022-11-04 11:40   ` [PATCH 1/3] bisect--helper: remove unused options Đoàn Trần Công Danh
2022-11-04 12:53     ` Jeff King
2022-11-04 11:40   ` [PATCH 2/3] bisect--helper: move all subcommands into their own functions Đoàn Trần Công Danh
2022-11-04 12:55     ` Jeff King
2022-11-04 13:32     ` Ævar Arnfjörð Bjarmason
2022-11-04 14:03       ` Đoàn Trần Công Danh
2022-11-04 11:40   ` [PATCH 3/3] bisect--helper: parse subcommand with OPT_SUBCOMMAND Đoàn Trần Công Danh
2022-11-04 13:00     ` Jeff King
2022-11-04 13:46     ` Ævar Arnfjörð Bjarmason
2022-11-04 14:07       ` Đoàn Trần Công Danh
2022-11-04 13:55   ` [PATCH 0/3] Convert git-bisect--helper to OPT_SUBCOMMAND Ævar Arnfjörð Bjarmason
2022-11-05 17:03   ` [PATCH v2 " Đoàn Trần Công Danh
2022-11-05 17:03     ` [PATCH v2 1/3] bisect--helper: remove unused options Đoàn Trần Công Danh
2022-11-05 17:03     ` [PATCH v2 2/3] bisect--helper: move all subcommands into their own functions Đoàn Trần Công Danh
2022-11-05 17:13       ` Đoàn Trần Công Danh
2022-11-05 17:03     ` [PATCH v2 3/3] bisect--helper: parse subcommand with OPT_SUBCOMMAND Đoàn Trần Công Danh
2022-11-05 17:07     ` [PATCH 00/13] Turn git-bisect to be builtin Đoàn Trần Công Danh
2022-11-05 17:07       ` [PATCH 01/13] bisect tests: test for v2.30.0 "bisect run" regressions Đoàn Trần Công Danh
2022-11-07 21:31         ` Ævar Arnfjörð Bjarmason
2022-11-08  1:17           ` Đoàn Trần Công Danh
2022-11-05 17:07       ` [PATCH 02/13] bisect: refactor bisect_run() to match CodingGuidelines Đoàn Trần Công Danh
2022-11-05 17:07       ` [PATCH 03/13] bisect--helper: pass arg[cv] down to do_bisect_run Đoàn Trần Công Danh
2022-11-05 17:07       ` [PATCH 04/13] bisect: fix output regressions in v2.30.0 Đoàn Trần Công Danh
2022-11-05 17:07       ` [PATCH 05/13] bisect run: keep some of the post-v2.30.0 output Đoàn Trần Công Danh
2022-11-07 21:40         ` Ævar Arnfjörð Bjarmason
2022-11-08  1:26           ` Đoàn Trần Công Danh
2022-11-08  3:11             ` Ævar Arnfjörð Bjarmason
2022-11-05 17:07       ` [PATCH 06/13] bisect--helper: remove unused arguments from do_bisect_run Đoàn Trần Công Danh
2022-11-05 17:07       ` [PATCH 07/13] bisect--helper: pretend we're real bisect when report error Đoàn Trần Công Danh
2022-11-07 21:29         ` Ævar Arnfjörð Bjarmason
2022-11-05 17:07       ` [PATCH 08/13] bisect test: test exit codes on bad usage Đoàn Trần Công Danh
2022-11-05 17:07       ` [PATCH 09/13] bisect--helper: emit usage for "git bisect" Đoàn Trần Công Danh
2022-11-05 17:07       ` [PATCH 10/13] bisect--helper: make `state` optional Đoàn Trần Công Danh
2022-11-05 17:07       ` [PATCH 11/13] bisect--helper: remove subcommand state Đoàn Trần Công Danh
2022-11-07 21:45         ` Ævar Arnfjörð Bjarmason
2022-11-08  1:27           ` Đoàn Trần Công Danh
2022-11-05 17:07       ` [PATCH 12/13] bisect--helper: log: allow arbitrary number of arguments Đoàn Trần Công Danh
2022-11-05 17:07       ` [PATCH 13/13] Turn `git bisect` into a full built-in Đoàn Trần Công Danh
2022-11-05 23:18     ` [PATCH v2 0/3] Convert git-bisect--helper to OPT_SUBCOMMAND Taylor Blau
2022-11-10 16:36   ` [PATCH v3 " Đoàn Trần Công Danh
2022-11-10 16:36     ` [PATCH v3 1/3] bisect--helper: remove unused options Đoàn Trần Công Danh
2022-11-11 12:42       ` Ævar Arnfjörð Bjarmason
2022-11-10 16:36     ` [PATCH v3 2/3] bisect--helper: move all subcommands into their own functions Đoàn Trần Công Danh
2022-11-11 13:51       ` Ævar Arnfjörð Bjarmason
2022-11-10 16:36     ` [PATCH v3 3/3] bisect--helper: parse subcommand with OPT_SUBCOMMAND Đoàn Trần Công Danh
2022-11-10 16:36     ` [PATCH v2 00/11] Turn git-bisect to be builtin Đoàn Trần Công Danh
2022-11-10 16:36       ` [PATCH v2 01/11] bisect tests: test for v2.30.0 "bisect run" regressions Đoàn Trần Công Danh
2022-11-10 16:36       ` [PATCH v2 02/11] bisect: refactor bisect_run() to match CodingGuidelines Đoàn Trần Công Danh
2022-11-10 16:36       ` [PATCH v2 03/11] bisect: fix output regressions in v2.30.0 Đoàn Trần Công Danh
2022-11-10 16:36       ` [PATCH v2 04/11] bisect run: keep some of the post-v2.30.0 output Đoàn Trần Công Danh
2022-11-10 16:36       ` [PATCH v2 05/11] bisect-run: verify_good: account for non-negative exit status Đoàn Trần Công Danh
2022-11-10 16:36       ` [PATCH v2 06/11] bisect--helper: identify as bisect when report error Đoàn Trần Công Danh
2022-11-10 16:36       ` [PATCH v2 07/11] bisect test: test exit codes on bad usage Đoàn Trần Công Danh
2022-11-10 16:36       ` [PATCH v2 08/11] bisect--helper: emit usage for "git bisect" Đoàn Trần Công Danh
2022-11-10 16:36       ` [PATCH v2 09/11] bisect--helper: handle states directly Đoàn Trần Công Danh
2022-11-10 16:36       ` [PATCH v2 10/11] bisect--helper: log: allow arbitrary number of arguments Đoàn Trần Công Danh
2022-11-11 14:01         ` Ævar Arnfjörð Bjarmason
2022-11-10 16:36       ` [PATCH v2 11/11] Turn `git bisect` into a full built-in Đoàn Trần Công Danh
2022-11-11 13:53         ` Ævar Arnfjörð Bjarmason
2022-11-11 15:37           ` Jeff King
2022-11-11 21:09             ` Ævar Arnfjörð Bjarmason
2022-11-11 22:07       ` [PATCH v2 00/11] Turn git-bisect to be builtin Taylor Blau
2022-11-15 19:18         ` Taylor Blau
2022-11-15 19:36           ` Jeff King
2022-11-15 19:40             ` Taylor Blau
2022-11-11 12:32     ` [PATCH v3 0/3] Convert git-bisect--helper to OPT_SUBCOMMAND Ævar Arnfjörð Bjarmason
2022-11-04 13:22 ` [PATCH 00/13] bisect: v2.30.0 "run" regressions + make it built-in Ævar Arnfjörð Bjarmason
2022-11-04 13:22   ` [PATCH 01/13] bisect tests: test for v2.30.0 "bisect run" regressions Ævar Arnfjörð Bjarmason
2022-11-04 13:22   ` [PATCH 02/13] bisect: refactor bisect_run() to match CodingGuidelines Ævar Arnfjörð Bjarmason
2022-11-04 13:22   ` [PATCH 03/13] bisect: fix output regressions in v2.30.0 Ævar Arnfjörð Bjarmason
2022-11-04 13:22   ` [PATCH 04/13] bisect run: fix "--log" eating regression " Ævar Arnfjörð Bjarmason
2022-11-04 13:22   ` [PATCH 05/13] bisect run: keep some of the post-v2.30.0 output Ævar Arnfjörð Bjarmason
2022-11-04 13:22   ` [PATCH 06/13] bisect test: test exit codes on bad usage Ævar Arnfjörð Bjarmason
2022-11-04 13:22   ` [PATCH 07/13] bisect--helper: emit usage for "git bisect" Ævar Arnfjörð Bjarmason
2022-11-04 13:22   ` [PATCH 08/13] bisect--helper: have all functions take state, argc, argv, prefix Ævar Arnfjörð Bjarmason
2022-11-04 13:22   ` [PATCH 09/13] parse-options API: don't restrict OPT_SUBCOMMAND() to one *_fn type Ævar Arnfjörð Bjarmason
2022-11-05  8:32     ` René Scharfe
2022-11-05 11:34       ` Đoàn Trần Công Danh
2022-11-05 21:32         ` Phillip Wood
2022-11-05 13:52       ` Ævar Arnfjörð Bjarmason
2022-11-05 16:36         ` Phillip Wood
2022-11-05 21:59           ` Ævar Arnfjörð Bjarmason
2022-11-05 17:26         ` René Scharfe
2022-11-05 22:33           ` Ævar Arnfjörð Bjarmason [this message]
2022-11-06  8:25             ` René Scharfe
2022-11-06 13:28               ` Ævar Arnfjörð Bjarmason
2022-11-12 10:42                 ` René Scharfe
2022-11-12 16:34                   ` Jeff King
2022-11-12 16:55                     ` Ævar Arnfjörð Bjarmason
2022-11-13 17:31                       ` René Scharfe
2022-11-04 13:22   ` [PATCH 10/13] bisect--helper: remove dead --bisect-{next-check,autostart} code Ævar Arnfjörð Bjarmason
2022-11-04 13:22   ` [PATCH 11/13] bisect--helper: convert to OPT_SUBCOMMAND_CB() Ævar Arnfjörð Bjarmason
2022-11-04 13:22   ` [PATCH 12/13] bisect--helper: make `state` optional Ævar Arnfjörð Bjarmason
2022-11-04 13:22   ` [PATCH 13/13] Turn `git bisect` into a full built-in Ævar Arnfjörð Bjarmason
2022-11-05  0:13   ` [PATCH 00/13] bisect: v2.30.0 "run" regressions + make it built-in Taylor Blau
2022-11-10 12:50   ` Johannes Schindelin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=221105.86o7tlvxh0.gmgdl@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=congdanhqx@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=l.s.r@web.de \
    --cc=ldoktor@redhat.com \
    --cc=me@ttaylorr.com \
    --cc=peff@peff.net \
    --cc=szeder.dev@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).