Am 10.08.23 um 02:41 schrieb Jeff King:

On Wed, Aug 09, 2023 at 06:41:13PM +0200, René Scharfe wrote:

quoted

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.

quoted

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), \