On Tue, Apr 13, 2021 at 11:08:19AM +0200, Ævar Arnfjörð Bjarmason wrote:
> In ee4512ed481 (trace2: create new combined trace facility,
> 2019-02-22) we started with two copies of this comment,
> 0ee10fd1296 (usage: add trace2 entry upon warning(), 2020-11-23) added
> a third. Let's instead add an earlier comment that applies to all
> these mostly-the-same functions.
I'm sometimes wary of this aggregating comments like this, because
somebody who is just reading the third function may not think to look
further up to find the comment.
But this comment in particular does not seem dangerous if somebody
misses it (unlike comments that are warning people about bad things
happening).
> + */
> static NORETURN void die_builtin(const char *err, 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(err, params);
>
> vreportf("fatal: ", err, params);
TBH, I am not sure it adds all that much value in the first place. It is
only telling the reader that the code is not broken. But I kind of
wonder if it should simply be doing the defensive thing anyway:
va_list cp;
va_copy(cp, params);
trace2_cmd_error_va(err, params);
va_end(cp);
We are relying on a subtle contract with the trace2 code, and there is
no compile-time check that it will be upheld (and indeed, on many
platforms we might not even notice if it isn't, depending on how va_list
is implemented). Looking at the trace2 code, we do not even enforce this
centrally. It is up to each target to remember to va_copy()!
Anyway, that is somewhat outside the scope of your series (though I
would not be sad to see this comment go away entirely in favor of the
more defensive code above).
-Peff
