Re: [RFC] CodingGuidelines: mark external declarations with "extern"

[Jeff King <peff@peff.net>]

On Fri, Oct 09, 2020 at 12:27:28PM -0700, Junio C Hamano wrote:

> The document recommends not to write "extern" in front of a function
> declaration, with justification that a decl by default is extern.
> 
> While it is true that by default decls are extern unless marked with
> "static", it does not justify the choice.  It only justifies why we
> could omit it if we wanted to, but does not say anything about the
> reason why we would want to omit it in the first place.

True, but I think that is perhaps a failing of the document, not of the
original motivation. The reason to have _a_ standard is that it is
confusing when some functions say "extern" and some do not (and it was a
continued source of review friction when people pointed out
inconsistencies). But the reason to standardize on omitting it is that
it clutters the line, making it longer and harder to read.

The argument for including it is less clear to me. You say below:

> [...]By doing so, we would also prevent a
> mistake of not writing "extern" when we need to (i.e. decls of data
> items, that are not functions) when less experienced developers try
> to mimic how the existing surrounding declarations are written.

but to my recollection that has not been a big problem. And it's one
that's usually easily caught by the compiler. A missing "extern" on a
variable will usually get you a multiple-definition warning at
link-time (if you manage to also omit the actual definition you won't
see that, though "make sparse" will warn that your variable ought to be
static).

So I have a mild preference to omit it, if we were starting from
scratch. But starting form where we are now, without "extern", my
preference is even stronger, because I don't want to waste submitter or
reviewer time switching all of the existing cases.

> But a function that returns a pointer to a function looks similar:
> 
>     extern void (*get_error_routine(void))(const char *message, ...);

IMHO the real problem here is that C's syntax for returning a function
pointer is so horrendous. How about this (on top of your earlier patch
to drop the extern from that declaration)?

-- >8 --
Subject: [PATCH] usage: define a type for a reporting function

The usage, die, warning, and error routines all work with a function
pointer that takes the message to be reported. We usually just mention
the function's full type inline. But this makes the use of these
pointers hard to read, especially because C's syntax for returning a
function pointer is so awful:

  void (*get_error_routine(void))(const char *err, va_list params);

Unless you read it very carefully, this looks like a function pointer
declaration. Let's instead use a single typedef to define a reporting
function, which is the same for all four types.

Signed-off-by: Jeff King <peff@peff.net>
---
I wondered if the compiler would complain about attaching a noreturn
attribute to an already-typedef'd pointer (rather than including it in
the typedef). gcc seems to be fine with it, but we could also define two
types, one with noreturn and one without.

 git-compat-util.h | 12 +++++++-----
 usage.c           | 18 +++++++++---------
 2 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 56e0e1e79d..adfea06897 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -489,11 +489,13 @@ static inline int const_error(void)
 #define error_errno(...) (error_errno(__VA_ARGS__), const_error())
 #endif
 
-void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));
-void set_error_routine(void (*routine)(const char *err, va_list params));
-void (*get_error_routine(void))(const char *err, va_list params);
-void set_warn_routine(void (*routine)(const char *warn, va_list params));
-void (*get_warn_routine(void))(const char *warn, va_list params);
+typedef void (*report_fn)(const char *, va_list params);
+
+void set_die_routine(NORETURN_PTR report_fn routine);
+void set_error_routine(report_fn routine);
+report_fn get_error_routine(void);
+void set_warn_routine(report_fn routine);
+report_fn get_warn_routine(void);
 void set_die_is_recursing_routine(int (*routine)(void));
 
 int starts_with(const char *str, const char *prefix);
diff --git a/usage.c b/usage.c
index 58fb5fff5f..06665823a2 100644
--- a/usage.c
+++ b/usage.c
@@ -108,33 +108,33 @@ static int die_is_recursing_builtin(void)
 
 /* If we are in a dlopen()ed .so write to a global variable would segfault
  * (ugh), so keep things static. */
-static NORETURN_PTR void (*usage_routine)(const char *err, va_list params) = usage_builtin;
-static NORETURN_PTR void (*die_routine)(const char *err, va_list params) = die_builtin;
-static void (*error_routine)(const char *err, va_list params) = error_builtin;
-static void (*warn_routine)(const char *err, va_list params) = warn_builtin;
+static NORETURN_PTR report_fn usage_routine = usage_builtin;
+static NORETURN_PTR report_fn die_routine = die_builtin;
+static report_fn error_routine = error_builtin;
+static report_fn warn_routine = warn_builtin;
 static int (*die_is_recursing)(void) = die_is_recursing_builtin;
 
-void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params))
+void set_die_routine(NORETURN_PTR report_fn routine)
 {
 	die_routine = routine;
 }
 
-void set_error_routine(void (*routine)(const char *err, va_list params))
+void set_error_routine(report_fn routine)
 {
 	error_routine = routine;
 }
 
-void (*get_error_routine(void))(const char *err, va_list params)
+report_fn get_error_routine(void)
 {
 	return error_routine;
 }
 
-void set_warn_routine(void (*routine)(const char *warn, va_list params))
+void set_warn_routine(report_fn routine)
 {
 	warn_routine = routine;
 }
 
-void (*get_warn_routine(void))(const char *warn, va_list params)
+report_fn get_warn_routine(void)
 {
 	return warn_routine;
 }
-- 
2.29.0.rc1.559.g850010a3b3