Jonathan Tan <jonathantanmy@xxxxxxxxxx> writes:
> 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);
