Re: [PATCH 2/2] C99 support: remove non-HAVE_VARIADIC_MACROS code

From: Jonathan Nieder <jrnieder@gmail.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	"brian m . carlson" <sandals@crustytoothpaste.net>,
	Jeff King <peff@peff.net>
Subject: Re: [PATCH 2/2] C99 support: remove non-HAVE_VARIADIC_MACROS code
Date: Thu, 20 May 2021 19:50:31 -0700	[thread overview]
Message-ID: <YKcf95uQ1W7COSsf@google.com> (raw)
In-Reply-To: <patch-2.2-f12e3cad57d-20210412T105422Z-avarab@gmail.com>

Hi,

Ævar Arnfjörð Bjarmason wrote:

> Remove code that depend on HAVE_VARIADIC_MACROS not being set. Since
> 765dc168882 (git-compat-util: always enable variadic macros,
> 2021-01-28) we've unconditionally defined it to be true, and that
> change went out with v2.31.0. This should have given packagers enough
> time to discover whether variadic macros were an issue.
>
> It seems that they weren't, so let's update the coding guidelines and
> remove all the fallback code for the non-HAVE_VARIADIC_MACROS case.

As discussed in the rest of this thread, that's a pretty short time,
so while I would love to be able to use variadic macros
unconditionally, I think we'd need a different rationale.

That said, I want us to be ready the moment external conditions allow.
Ideally we want it to be as simple as

	git grep --name-only -e HAVE_VARIADIC_MACROS |
	xargs unifdef -m -DHAVE_VARIADIC_MACROS=1

plus removing the #define; is there anything we need to do in advance
to make that work well?  Let's see.

[...]
> --- a/trace.h
> +++ b/trace.h
> @@ -126,66 +126,6 @@ void trace_command_performance(const char **argv);
>  void trace_verbatim(struct trace_key *key, const void *buf, unsigned len);
>  uint64_t trace_performance_enter(void);
>  
> -#ifndef HAVE_VARIADIC_MACROS
> -
> -/**
> - * Prints a formatted message, similar to printf.
> - */
> -__attribute__((format (printf, 1, 2)))
> -void trace_printf(const char *format, ...);

This removes the documentation for these functions and doesn't add it
back on the #else side.  So I think we'd want the following
preparatory patch.

Thanks,
Jonathan

-- >8 --
Subject: trace: move comments to variadic macro variant of trace functions

Nowadays compilers not having variadic macros are the exception.  Move
API documentation to the declarations used in the common case.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 trace.h | 79 +++++++++++++++++++++++++++------------------------------
 1 file changed, 38 insertions(+), 41 deletions(-)

diff --git a/trace.h b/trace.h
index 0dbbad0e41..fc7eb0bc72 100644
--- a/trace.h
+++ b/trace.h
@@ -128,56 +128,22 @@ uint64_t trace_performance_enter(void);
 
 #ifndef HAVE_VARIADIC_MACROS
 
-/**
- * Prints a formatted message, similar to printf.
- */
+/* Fallback implementation that does not add file:line. */
+
 __attribute__((format (printf, 1, 2)))
 void trace_printf(const char *format, ...);
 
 __attribute__((format (printf, 2, 3)))
 void trace_printf_key(struct trace_key *key, const char *format, ...);
 
-/**
- * Prints a formatted message, followed by a quoted list of arguments.
- */
 __attribute__((format (printf, 2, 3)))
 void trace_argv_printf(const char **argv, const char *format, ...);
 
-/**
- * Prints the strbuf, without additional formatting (i.e. doesn't
- * choke on `%` or even `\0`).
- */
 void trace_strbuf(struct trace_key *key, const struct strbuf *data);
 
-/**
- * Prints elapsed time (in nanoseconds) if GIT_TRACE_PERFORMANCE is enabled.
- *
- * Example:
- * ------------
- * uint64_t t = 0;
- * for (;;) {
- * 	// ignore
- * t -= getnanotime();
- * // code section to measure
- * t += getnanotime();
- * // ignore
- * }
- * trace_performance(t, "frotz");
- * ------------
- */
 __attribute__((format (printf, 2, 3)))
 void trace_performance(uint64_t nanos, const char *format, ...);
 
-/**
- * Prints elapsed time since 'start' if GIT_TRACE_PERFORMANCE is enabled.
- *
- * Example:
- * ------------
- * uint64_t start = getnanotime();
- * // code section to measure
- * trace_performance_since(start, "foobar");
- * ------------
- */
 __attribute__((format (printf, 2, 3)))
 void trace_performance_since(uint64_t start, const char *format, ...);
 
@@ -186,11 +152,6 @@ void trace_performance_leave(const char *format, ...);
 
 #else
 
-/*
- * Macros to add file:line - see above for C-style declarations of how these
- * should be used.
- */
-
 /*
  * TRACE_CONTEXT may be set to __FUNCTION__ if the compiler supports it. The
  * default is __FILE__, as it is consistent with assert(), and static function
@@ -227,8 +188,14 @@ void trace_performance_leave(const char *format, ...);
 					    __VA_ARGS__);		    \
 	} while (0)
 
+/**
+ * Prints a formatted message, similar to printf.
+ */
 #define trace_printf(...) trace_printf_key(&trace_default_key, __VA_ARGS__)
 
+/**
+ * Prints a formatted message, followed by a quoted list of arguments.
+ */
 #define trace_argv_printf(argv, ...)					    \
 	do {								    \
 		if (trace_pass_fl(&trace_default_key))			    \
@@ -236,12 +203,32 @@ void trace_performance_leave(const char *format, ...);
 					    argv, __VA_ARGS__);		    \
 	} while (0)
 
+/**
+ * Prints the strbuf, without additional formatting (i.e. doesn't
+ * choke on `%` or even `\0`).
+ */
 #define trace_strbuf(key, data)						    \
 	do {								    \
 		if (trace_pass_fl(key))					    \
 			trace_strbuf_fl(TRACE_CONTEXT, __LINE__, key, data);\
 	} while (0)
 
+/**
+ * Prints elapsed time (in nanoseconds) if GIT_TRACE_PERFORMANCE is enabled.
+ *
+ * Example:
+ * ------------
+ * uint64_t t = 0;
+ * for (;;) {
+ * 	// ignore
+ * t -= getnanotime();
+ * // code section to measure
+ * t += getnanotime();
+ * // ignore
+ * }
+ * trace_performance(t, "frotz");
+ * ------------
+ */
 #define trace_performance(nanos, ...)					    \
 	do {								    \
 		if (trace_pass_fl(&trace_perf_key))			    \
@@ -249,6 +236,16 @@ void trace_performance_leave(const char *format, ...);
 					     __VA_ARGS__);		    \
 	} while (0)
 
+/**
+ * Prints elapsed time since 'start' if GIT_TRACE_PERFORMANCE is enabled.
+ *
+ * Example:
+ * ------------
+ * uint64_t start = getnanotime();
+ * // code section to measure
+ * trace_performance_since(start, "foobar");
+ * ------------
+ */
 #define trace_performance_since(start, ...)				    \
 	do {								    \
 		if (trace_pass_fl(&trace_perf_key))			    \
-- 
2.31.1.818.g46aad6cb9e

