From: "René Scharfe" <l.s.r@web.de>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>, git@vger.kernel.org
Cc: "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, 5 Nov 2022 09:32:44 +0100 [thread overview]
Message-ID: <df855ba1-52b1-1007-68e8-2e28e85b6822@web.de> (raw)
In-Reply-To: <patch-09.13-2cb3807aa17-20221104T132117Z-avarab@gmail.com>
Am 04.11.22 um 14:22 schrieb Ævar Arnfjörð Bjarmason:
> When the OPT_SUBCOMMAND() API was implemented in [1] it did so by
> adding a new "subcommand_fn" member to "struct option", rather than
> allowing the user of the API to pick the type of the function.
>
> An advantage of mandating that "parse_opt_subcommand_fn" must be used
> is that we'll get type checking for the function we're passing in, a
> disadvantage is that we can't convert e.g. "builtin/bisect--helper.c"
> easily to it, as its callbacks need their own argument.
>
> Let's generalize this interface, while leaving in place a small hack
> to give the existing API users their type safety. We assign to
> "typecheck_subcommand_fn", but don't subsequently use it for
> anything. Instead we use the "defval" and "value" members.
>
> A subsequent commit will add a OPT_SUBCOMMAND() variant where the
> "callback" isn't our default "parse_options_pick_subcommand" (and that
> caller won't be able to use the type checking).
>
> 1. fa83cc834da (parse-options: add support for parsing subcommands,
> 2022-08-19)
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> parse-options.c | 9 ++++++---
> parse-options.h | 25 +++++++++++++++++++++----
> 2 files changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/parse-options.c b/parse-options.c
> index a1ec932f0f9..1d9e46c9dc7 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -427,7 +427,8 @@ static enum parse_opt_result parse_subcommand(const char *arg,
> for (; options->type != OPTION_END; options++)
> if (options->type == OPTION_SUBCOMMAND &&
> !strcmp(options->long_name, arg)) {
> - *(parse_opt_subcommand_fn **)options->value = options->subcommand_fn;
> + if (options->callback(options, arg, 0))
> + BUG("OPT_SUBCOMMAND callback returning non-zero");
> return PARSE_OPT_SUBCOMMAND;
> }
>
> @@ -506,8 +507,10 @@ static void parse_options_check(const struct option *opts)
> "That case is not supported yet.");
> break;
> case OPTION_SUBCOMMAND:
> - if (!opts->value || !opts->subcommand_fn)
> - optbug(opts, "OPTION_SUBCOMMAND needs a value and a subcommand function");
> + if (!opts->value || !opts->callback)
> + optbug(opts, "OPTION_SUBCOMMAND needs a value and a callback function");
> + if (opts->ll_callback)
> + optbug(opts, "OPTION_SUBCOMMAND uses callback, not ll_callback");
> if (!subcommand_value)
> subcommand_value = opts->value;
> else if (subcommand_value != opts->value)
> diff --git a/parse-options.h b/parse-options.h
> index b6ef86e0d15..61e3016c3fc 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -128,19 +128,24 @@ typedef int parse_opt_subcommand_fn(int argc, const char **argv,
> * the option takes optional argument.
> *
> * `callback`::
> - * pointer to the callback to use for OPTION_CALLBACK
> + * pointer to the callback to use for OPTION_CALLBACK and OPTION_SUBCOMMAND.
> *
> * `defval`::
> * default value to fill (*->value) with for PARSE_OPT_OPTARG.
> * OPTION_{BIT,SET_INT} store the {mask,integer} to put in the value when met.
> + * OPTION_SUBCOMMAND stores the pointer the function selected for
> + * the subcommand.
> + *
> * CALLBACKS can use it like they want.
> *
> * `ll_callback`::
> * pointer to the callback to use for OPTION_LOWLEVEL_CALLBACK
> *
> * `subcommand_fn`::
> - * pointer to a function to use for OPTION_SUBCOMMAND.
> - * It will be put in value when the subcommand is given on the command line.
> + * pointer to the callback used with OPT_SUBCOMMAND() and
> + * OPT_SUBCOMMAND_F(). Internally we store the same value in
> + * `defval`. This is only here to give the OPT_SUBCOMMAND{,_F}()
> + * common case type safety.
> */
> struct option {
> enum parse_opt_type type;
> @@ -217,12 +222,24 @@ struct option {
> #define OPT_ALIAS(s, l, source_long_name) \
> { OPTION_ALIAS, (s), (l), (source_long_name) }
>
> +static inline int parse_options_pick_subcommand_cb(const struct option *option,
> + const char *arg UNUSED,
> + int unset UNUSED)
> +{
> + parse_opt_subcommand_fn *fn = (parse_opt_subcommand_fn *)option->defval;
> + *(parse_opt_subcommand_fn **)option->value = fn;
->defval is of type intptr_t and ->value is a void pointer. The result
of converting a void pointer value to an intptr_t and back is a void
pointer equal to the original pointer if I read 6.3.2.3 (Pointers,
paragraphs 5 and 6) and 7.18.1.4 (Integer types capable of holding
object pointers) in C99 correctly.
6.3.2.3 paragraph 8 says that casting between function pointers of
different type is OK and you can get your original function pointer
back and use it in a call if you convert it back to the right type.
Casting between a function pointer and an object pointer is undefined,
though. They don't have to be of the same size, so a function pointer
doesn't have to fit into an intptr_t. I wouldn't be surprised if CHERI
(https://www.cl.cam.ac.uk/research/security/ctsrd/cheri/) was an actual
example of that.
Why is this trickery needed? Above you write that callbacks in
builtin/bisect--helper.c can't use subcommand_fn because they need
their own argument. Can we extend subcommand_fn or use a global
variable to pass that extra thing instead? The latter may be ugly, but
at least it's valid C..
> + return 0;
> +}
> +
> #define OPT_SUBCOMMAND_F(l, v, fn, f) { \
> .type = OPTION_SUBCOMMAND, \
> .long_name = (l), \
> .value = (v), \
> .flags = (f), \
> - .subcommand_fn = (fn) }
> + .defval = (intptr_t)(fn), \
> + .subcommand_fn = (fn), \
> + .callback = parse_options_pick_subcommand_cb, \
Getting the address of an inline function feels weird, but the compiler
is free to emit to ignore that keyword and will provide an addressable
function object here.
> +}
> #define OPT_SUBCOMMAND(l, v, fn) OPT_SUBCOMMAND_F((l), (v), (fn), 0)
>
> /*
next prev parent reply other threads:[~2022-11-05 8:33 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 [this message]
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
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=df855ba1-52b1-1007-68e8-2e28e85b6822@web.de \
--to=l.s.r@web.de \
--cc=Johannes.Schindelin@gmx.de \
--cc=avarab@gmail.com \
--cc=congdanhqx@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--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).