From: "René Scharfe" <l.s.r@web.de>
To: Jeff King <peff@peff.net>
Cc: Junio C Hamano <gitster@pobox.com>, Git List <git@vger.kernel.org>
Subject: Re: [PATCH] describe: fix --no-exact-match
Date: Thu, 10 Aug 2023 21:10:33 +0200 [thread overview]
Message-ID: <09f499ad-28a5-0d8b-8af6-97475bdc614b@web.de> (raw)
In-Reply-To: <20230810004127.GD795985@coredump.intra.peff.net>
Am 10.08.23 um 02:41 schrieb Jeff King:
> On Wed, Aug 09, 2023 at 06:41:13PM +0200, René Scharfe wrote:
>
>> And sorry for the unused parameter warning. Just checked; there are
>> 170+ of those remaining before we can enable it in developer mode. :-/
>> Seems worthwhile, though, especially not slapping UNUSED blindly on
>> them.
>
> I know, I'm working on it. :) There were more than 800 when I started.
> I'm hoping to send out the final patches during this 2.43 cycle.
>
>> Oh. Do we really need all those? Anyway, if we added at least the
>> most common ones, we'd be better off already, I'd expect:
>>
>> $ % git grep -h ' = opt->value;' | sed 's/\*.*$//; s/^ *//' | sort | uniq -c | sort -nr | head -10
>> 29 struct diff_options
>> 12 int
>> 7 struct grep_opt
>> 6 struct rebase_options
>> 6 struct apply_state
>> 5 struct strbuf
>> 5 char
>> 4 struct note_data
>> 3 struct string_list
>> 2 struct strvec
>>
>> Increasing the size of the struct like that would increase the total
>> memory footprint by a few KB at most -- tolerable.
>
> Hmm, I was thinking that "int_value" might not be so bad. But it seems
> like a pretty bad layering violation for parse-options.c to know about
> "struct apply_state" and so on. That really is what void pointers are
> for.
True, keeping a list of external types in parse-options.h is clumsy and
ugly.
> As for size, you should be able to use a union. All of the types inside
> the struct are pointers, so they'd all be the same size. Of course then
> you lose some of the safety. If the caller assigned to "int_value" that
> is distinct from "foo_value", then you can be sure you will get a NULL
> and segfault upon accessing foo_value. With a union, you get whatever
> type-punning undefined behavior the compiler sees fit. And the point is
> making sure the two match.
Which makes a union a non-starter -- we need a reliable error.
> We really don't care about separate storage, though. We want type
> safety. Maybe an option there would be to let the callback function take
> the appropriate type, and cast it. I.e., something like:
>
> /* define a callback taking the real type */
> int my_int_opt(int *value, struct option *opt, ...etc...) { ...body... }
>
> /* when mentioning a callback, include the type, and make sure that
> * the value and the callback both match it */
> #define OPT_CALLBACK(s, l, type, v, cb, ...etc...) \
> { ..., \
> value.type = (v), \
> callback = (int (*)(type *, struct option *opt, ...etc...), \
> }
> ...
> OPT_CALLBACK('f', "foo", int, &my_local_int, my_int_opt)
The idea is good, even though I don't understand how your specific
example would work. Dabbled with something similar for typed qsort
(need to clean it up and submit it one of these days).
> I'm pretty sure that falls afoul of the standard, though, because we
> eventually need to call that function, and we'll do so through a
> function pointer that doesn't match its declaration.
Fighting possible type mismatches by adding a certain type mismatch
won't fly..
> I'm not sure there's a portable and non-insane way of doing what we want
> here. At least at compile-time.
We need a wrapper with the correct signature. The wrapper is plugged
into struct option. The typed callback is called by the wrapper and
can be used for a type check in the struct macro. Demo patch below.
> At run-time you could record type
> information in an enum and check it in the callback. That seems like
> a lot of work for little reward, though.
Agreed -- runtime type checks are for scripting languages..
René
diff --git a/builtin/describe.c b/builtin/describe.c
index b28a4a1f82..ce16c36de2 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -558,15 +558,17 @@ static void describe(const char *arg, int last_one)
strbuf_release(&sb);
}
-static int option_parse_exact_match(const struct option *opt, const char *arg,
- int unset)
+static int option_parse_exact_match(const struct option *opt UNUSED,
+ const char *arg, int unset, int *value)
{
BUG_ON_OPT_ARG(arg);
- max_candidates = unset ? DEFAULT_CANDIDATES : 0;
+ *value = unset ? DEFAULT_CANDIDATES : 0;
return 0;
}
+DEFINE_PARSE_OPT_CB(option_parse_exact_match);
+
int cmd_describe(int argc, const char **argv, const char *prefix)
{
int contains = 0;
@@ -578,9 +580,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
OPT_BOOL(0, "long", &longformat, N_("always use long format")),
OPT_BOOL(0, "first-parent", &first_parent, N_("only follow first parent")),
OPT__ABBREV(&abbrev),
- OPT_CALLBACK_F(0, "exact-match", NULL, NULL,
- N_("only output exact matches"),
- PARSE_OPT_NOARG, option_parse_exact_match),
+ OPT_CALLBACK_F_T(0, "exact-match", &max_candidates, NULL,
+ N_("only output exact matches"),
+ PARSE_OPT_NOARG, option_parse_exact_match),
OPT_INTEGER(0, "candidates", &max_candidates,
N_("consider <n> most recent tags (default: 10)")),
OPT_STRING_LIST(0, "match", &patterns, N_("pattern"),
diff --git a/parse-options.h b/parse-options.h
index 57a7fe9d91..f957931cfa 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -67,6 +67,14 @@ enum parse_opt_result {
struct option;
typedef int parse_opt_cb(const struct option *, const char *arg, int unset);
+#define DEFINE_PARSE_OPT_CB(name) \
+static inline int name ## __void(const struct option *opt, \
+ const char *arg, int unset) \
+{ \
+ return name(opt, arg, unset, opt->value); \
+} \
+struct option
+
struct parse_opt_ctx_t;
typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
const struct option *opt,
@@ -198,6 +206,16 @@ struct option {
.flags = (f), \
.callback = (cb), \
}
+#define OPT_CALLBACK_F_T(s, l, v, a, h, f, cb) { \
+ .type = OPTION_CALLBACK, \
+ .short_name = (s) + (0 ? cb(NULL, NULL, 0, (v)) : 0), \
+ .long_name = (l), \
+ .value = (v), \
+ .argh = (a), \
+ .help = (h), \
+ .flags = (f), \
+ .callback = cb ## __void, \
+}
#define OPT_STRING_F(s, l, v, a, h, f) { \
.type = OPTION_STRING, \
.short_name = (s), \
next prev parent reply other threads:[~2023-08-10 19:10 UTC|newest]
Thread overview: 66+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-18 15:44 [PATCH] ls-tree: fix --no-full-name René Scharfe
2023-07-18 16:37 ` Junio C Hamano
2023-07-18 20:49 ` Junio C Hamano
2023-07-21 12:41 ` René Scharfe
2023-07-21 12:41 ` René Scharfe
2023-07-21 14:37 ` Junio C Hamano
2023-07-21 19:29 ` René Scharfe
2023-07-21 20:09 ` Junio C Hamano
2023-07-21 20:14 ` Junio C Hamano
2023-07-24 12:29 ` René Scharfe
2023-07-24 18:51 ` Junio C Hamano
2023-07-24 20:09 ` René Scharfe
2023-07-24 20:50 ` Junio C Hamano
2023-07-28 6:12 ` René Scharfe
2023-07-28 9:45 ` Phillip Wood
2023-07-29 20:40 ` René Scharfe
2023-07-31 15:31 ` Junio C Hamano
2023-08-04 16:40 ` Junio C Hamano
2023-08-04 19:48 ` Phillip Wood
2023-08-05 10:40 ` René Scharfe
2023-07-24 12:29 ` [PATCH v2 0/5] show negatability of options in short help René Scharfe
2023-07-24 12:34 ` [PATCH v2 1/5] subtree: disallow --no-{help,quiet,debug,branch,message} René Scharfe
2023-07-24 12:36 ` [PATCH v2 2/5] t1502, docs: disallow --no-help René Scharfe
2023-07-24 12:38 ` [PATCH v2 3/5] t1502: move optionspec help output to a file René Scharfe
2023-07-24 12:39 ` [PATCH v2 4/5] t1502: test option negation René Scharfe
2023-07-24 12:40 ` [PATCH v2 5/5] parse-options: show negatability of options in short help René Scharfe
2023-08-05 14:33 ` [PATCH v3 0/8] " René Scharfe
2023-08-05 14:37 ` [PATCH v3 1/8] subtree: disallow --no-{help,quiet,debug,branch,message} René Scharfe
2023-08-05 14:37 ` [PATCH v3 2/8] t1502, docs: disallow --no-help René Scharfe
2023-08-05 14:38 ` [PATCH v3 3/8] t1502: move optionspec help output to a file René Scharfe
2023-08-05 14:39 ` [PATCH v3 4/8] t1502: test option negation René Scharfe
2023-08-05 14:40 ` [PATCH v3 5/8] parse-options: show negatability of options in short help René Scharfe
2023-08-05 14:43 ` [PATCH v3 6/8] parse-options: factor out usage_indent() and usage_padding() René Scharfe
2023-08-05 14:44 ` [PATCH v3 7/8] parse-options: no --[no-]no- René Scharfe
2023-08-05 14:52 ` [PATCH v3 8/8] parse-options: simplify usage_padding() René Scharfe
2023-08-05 23:04 ` Junio C Hamano
2023-07-21 12:41 ` [PATCH] show-branch: fix --no-sparse René Scharfe
2023-07-21 14:42 ` Junio C Hamano
2023-07-21 16:30 ` René Scharfe
2023-07-21 12:41 ` [PATCH] show-branch: disallow --no-{date,topo}-order René Scharfe
2023-07-21 12:41 ` [PATCH] reset: disallow --no-{mixed,soft,hard,merge,keep} René Scharfe
2023-07-21 12:41 ` [PATCH] pack-objects: fix --no-quiet René Scharfe
2023-07-21 12:41 ` [PATCH] pack-objects: fix --no-keep-true-parents René Scharfe
2023-07-21 17:03 ` Junio C Hamano
2023-07-21 12:42 ` [PATCH] branch: disallow --no-{all,remotes} René Scharfe
2023-07-21 12:42 ` [PATCH] am: unify definition of --keep-cr and --no-keep-cr René Scharfe
2023-07-21 13:41 ` [PATCH] describe: fix --no-exact-match René Scharfe
2023-07-21 14:10 ` Junio C Hamano
2023-07-21 17:00 ` Junio C Hamano
2023-08-08 21:27 ` Jeff King
2023-08-08 21:28 ` Jeff King
2023-08-09 1:43 ` Junio C Hamano
2023-08-09 14:09 ` Jeff King
2023-08-09 16:41 ` René Scharfe
2023-08-09 19:07 ` Junio C Hamano
2023-08-10 0:26 ` Jeff King
2023-08-10 1:00 ` Junio C Hamano
2023-08-10 19:45 ` René Scharfe
2023-08-10 0:41 ` Jeff King
2023-08-10 19:10 ` René Scharfe [this message]
2023-08-11 15:11 ` Jeff King
2023-08-11 17:59 ` René Scharfe
2023-08-11 18:24 ` Jeff King
2023-08-12 5:11 ` René Scharfe
2023-08-11 15:13 ` Jeff King
2023-08-11 17:59 ` René Scharfe
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=09f499ad-28a5-0d8b-8af6-97475bdc614b@web.de \
--to=l.s.r@web.de \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=peff@peff.net \
/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).