Re: [PATCH v2 2/2] C99: remove hardcoded-out !HAVE_VARIADIC_MACROS code

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 28, 2022, 22:40 UTC
Message-ID: <xmqq4k5nwkl5.fsf@gitster.g>
In-Reply-To: <patch-v2-2.2-966d96505cb-20220128T110330Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 5 quoted lines

> Remove the "else" branches of the HAVE_VARIADIC_MACROS macro, which
> has been unconditionally omitted since 765dc168882 (git-compat-util:
> always enable variadic macros, 2021-01-28).
>
> Since they were hardcoded out anyone trying to compile a version of

"compile" -> "use a compiler without variadic macro support to compile"

> git v2.31.0 or later would have had a compilation error. 10 months
> across a few releases since then should have been enough time for
> anyone who cared to run into that and report the issue.

OK.

The verb "hardcode out" sounds iffy to me, but the reasoning above and all the additions and removals in in this patch look quite sensible.

Some paragraphs that merely got moved probably needs more work, though.

Show 20 quoted lines

>  Documentation/CodingGuidelines |   3 +
>  banned.h                       |   5 --
>  git-compat-util.h              |  12 ---
>  trace.c                        |  80 +-------------------
>  trace.h                        | 133 +++++++++++++--------------------
>  trace2.c                       |  39 ----------
>  trace2.h                       |  25 -------
>  usage.c                        |  15 +---
>  8 files changed, 58 insertions(+), 254 deletions(-)
>
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> index 0e27b5395d8..1858075a159 100644
> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -210,6 +210,9 @@ For C programs:
>     . since mid 2017 with 512f41cf, we have been using designated
>       initializers for array (e.g. "int array[10] = { [5] = 2 }").
>  
> +   . since early 2021 with 765dc168882, we have been using variadic
> +     macros, mostly for printf-like trace and debug macros.

OK.

Show 12 quoted lines

> @@ -218,7 +143,23 @@ void trace_performance_leave(const char *format, ...);
>   *
>   * which is invalid (note the ',)'). With GNUC, '##__VA_ARGS__' drops the
>   * comma, but this is non-standard.
> + *
> + * 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
> + * names are not necessarily unique.
> + *
> + * __FILE__ ":" __FUNCTION__ doesn't work with GNUC, as __FILE__ is supplied
> + * by the preprocessor as a string literal, and __FUNCTION__ is filled in by
> + * the compiler as a string constant.

Want to say that it is a possibility to use pasted string literal that involves __FILE__ and __LINE__ here?

> + */
> +#ifndef TRACE_CONTEXT
> +# define TRACE_CONTEXT __FILE__
> +#endif

Show 5 quoted lines

> +
> +/**
> + * Prints a formatted message, similar to printf.
>   */
> +#define trace_printf(...) trace_printf_key(&trace_default_key, __VA_ARGS__)

OK.

> +/**
> + * Prints a formatted message, followed by a quoted list of arguments.
> + */

This comment was OK in front of the declaration of the function version of this thing, i.e.

    void trace_argv_printf(const char **argv, const char *format, ...);

because the function prototype made it clear what type the initial parameters are.

But it is way too inadequate for the varargs macro version of this function, since ...

>  #define trace_argv_printf(argv, ...)					    \

... it is harder to tell what argv is without type and impossible to know the next one is a format string. Just moving it around is not enough; it needs to be a bit more helpful.

Show 8 quoted lines

> @@ -236,12 +178,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`).
> + */

Likewise.  The function version was declared as

	trace_strbuf(struct trace_key *key, const struct strbuf *data)

so mention of "strbuf" was sufficient to tell that the second parameter is what gets printed. With the macro version, the comment needs to make an extra effort, as the reader has less clue to work with, compared to the function version.

Thanks.