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

From: Phillip Wood <phillip.wood123@gmail.com>
To: "Đoàn Trần Công Danh" <congdanhqx@gmail.com>,
	"René Scharfe" <l.s.r@web.de>
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	git@vger.kernel.org, "Junio C Hamano" <gitster@pobox.com>,
	"Jeff King" <peff@peff.net>, "Taylor Blau" <me@ttaylorr.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 21:32:36 +0000	[thread overview]
Message-ID: <43ec357b-bbd2-6b17-58f4-676a8c5e1749@dunelm.org.uk> (raw)
In-Reply-To: <Y2ZKHPkEXViv9wcp@danh.dev>

Hi Đoàn

On 05/11/2022 11:34, Đoàn Trần Công Danh wrote:
>   
> On 2022-11-05 09:32:44+0100, René Scharfe <l.s.r@web.de> wrote:
>> 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..
> 
> Not the author, but I fully agree with you, I think instead of adding new API
> for some arbitrary subcommand_fn, I would change the subcommand_fn to
> type:
> 
> 	int (*)(int argc, const char **argv, const char *prefix, void *context)
> 
> The last argument would be an object pointer, which will be casted to
> the correct type inside the callback.
> 
> Let me cherry-picking this series on top of mine to see how things
> would progress.

Unfortunately the current implementation of OPT_SUBCOMMAND relies on 
returning a function pointer though a void* variable which while widely 
supported is undefined behavior. I wonder if an API like

typedef int (*subcommand_fn)(int, char**, char*);
typedef int (*subcommand_ctk_fn)(int, char**, char*, void*);
struct subcommand; /* opaque */

OPT_SUBCOMMAND(name, value, func);
OPT_SUMCOMMAND_CTX(name, value, func, ctx);

int call_subcommand(struct subcommand* subcommand, int argc, char** 
argv, char* prefix);

which would be used as

int sub1_fn(int argc, char** argv, char* prefix, void* ctx)
{
	struct cmd_ctx cmd_ctx = ctx
	...
}

int sub2_fn(int argc, char** argv, char* prefix)
{
	...
}

int cmd_foo(int argc, char** argv, char* prefix)
{
	struct cmd_ctx ctx = ...
	struct subcommand cmd = { 0 };
	...
	struct option opts[] = {
		OPT_SUBCOMMAND_CTX("sub1", &cmd, sub_fn, &ctx);
		OPT_SUBCOMMAND("sub2", &cmd, sub2_fn);
	};
	argc = parse_options(argc, argv, prefix, &opts, usage, flags);
	return call_subcommand(&cmd, argc, argv, prefix);
}

would be an improvement. One can avoid having to mark the ctx parameter 
as UNUSED() if a subcommand does not need any context by using 
OPT_SUBCOMMAND() rather than OPT_SUBCOMMAND_CTX().

The implementation of call_subcommand() would look something like

struct subcommand {
         void* ctx;
         int has_ctx;
         union {
                 subcommand_fn fn;
                 subcommand_ctx_fn ctx_fn;
         };
};

int call_subcommand(struct subcommand* subcommand, int argc, char 
**argv, char* prefix)
{
         if (subcommand->has_ctx)
                 return subcommand->ctx_fn(argc, argv, prefix, 
subcommand->ctx);
         else
                 return subcommand->fn(argc, argv, prefix);
}

Best Wishes

Phillip