Re: [RFC PATCH v2] usage: add trace2 entry upon warning()
From: Junio C Hamano <hidden>
Date: 2020-11-23 22:30:39
Jonathan Tan [off-list ref] writes:
quoted hunk
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);