Re: [RFC PATCH v2] usage: add trace2 entry upon warning()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 23, 2020, 22:30 UTC
- Message-ID
- <xmqqblfnn1u3.fsf@gitster.c.googlers.com>
- In-Reply-To
- <20201123204522.675836-1-jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
Show 15 quoted lines
> diff --git a/usage.c b/usage.c
> index 06665823a2..1868a24f7a 100644
> --- a/usage.c
> +++ b/usage.c
> @@ -81,6 +81,12 @@ static void error_builtin(const char *err, va_list params)
>
> static void warn_builtin(const char *warn, va_list params)
> {
> + /*
> + * We call this trace2 function first and expect it to va_copy 'params'
> + * before using it (because an 'ap' can only be walked once).
> + */
> + trace2_cmd_error_va(warn, params);
> +
> vreportf("warning: ", warn, params);Without the comment, this would have been a huge red sign. Given what the trace2 function does, I think cmd_error_va() has to do the va_copy()/va_end() dance somewhere in it, so from the caller's point of view, not having to va_copy()/va_end() is convenient and nice, but perhaps we can eventually (not today---we have the same issue with die_builtin() and error_builtin()) move the comment to the callee so that other people who are writing more callers do not have to rediscover what to do with "can va_lsit be used more than once?" issue. Perhaps like (I am not demonstrating the removal of dups):
trace2.h | 5 +++++ 1 file changed, 5 insertions(+)
diff --git c/trace2.h w/trace2.h index b18bc5529c..10ba231122 100644 --- c/trace2.h +++ w/trace2.h @@ -116,6 +116,11 @@ int trace2_cmd_exit_fl(const char *file, int line, int code); * Emit an 'error' event. * * Write an error message to the TRACE2 targets. + * + * Note that the "va_list ap" the caller has is not touched, because + * it is fed to each of the TRACE2 targets, which must use va_copy() + * and use va_end() on the copy. The caller can still use ap it uses + * to call this funciton and and call va_end() on it when it is done. */ void trace2_cmd_error_va_fl(const char *file, int line, const char *fmt, va_list ap);