| Message ID | patch-v3-3.3-27ea260bbea-20220219T103752Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | C99: remove dead !HAVE_VARIADIC_MACROS code | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > The definition of "TRACE_CONTEXT" as "__FILE__" added in > e05bed960d3 (trace: add 'file:line' to all trace output, 2014-07-12) > appeared between v6[1] and v7[2] of the series that added it. > > It does not appear to have been something anybody asked for, and > doesn't seem to have been used by anyone since then to override it to > something other than the default __FILE__. Sorry, but I do not quite follow. How can we claim nobody used it, given that it should be usable with CFLAGS=-DTRACE_CONTEXT=ANYTHING? On the other hand, I do not see how it is hurting anybody to have this indirection today. Or am I missing something obvious?
On Sun, Feb 20 2022, Junio C Hamano wrote: [I forgot to CC Karsten Blees] > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> The definition of "TRACE_CONTEXT" as "__FILE__" added in >> e05bed960d3 (trace: add 'file:line' to all trace output, 2014-07-12) >> appeared between v6[1] and v7[2] of the series that added it. >> >> It does not appear to have been something anybody asked for, and >> doesn't seem to have been used by anyone since then to override it to >> something other than the default __FILE__. > > Sorry, but I do not quite follow. How can we claim nobody used it, > given that it should be usable with CFLAGS=-DTRACE_CONTEXT=ANYTHING? > > On the other hand, I do not see how it is hurting anybody to have > this indirection today. > > Or am I missing something obvious? I don't know that, but: * From the list discussion at the time it seems nobody asked for this, it was just something the author "snuck in" between re-rolls. * I didn't find any subsequent on-list discussion of people actually using this. Both leads me to believe that this was a neat-at-the-time but never used for anything outside one-off testing feature. Perhaps you'd like a v4 without this. It isn't strictly needed, but where I'm going with this series is improving the usage.c output/passing of these __{FILE,LINE,FUNCTION}__. I don't need it now, but if we'd eventually like to unify trace & trace2 we'll need to deal with this part, since those functions won't deal well with replacing a __FILE__ "just pass whatever data you'd like here".
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Perhaps you'd like a v4 without this. It isn't strictly needed, but > where I'm going with this series is improving the usage.c output/passing > of these __{FILE,LINE,FUNCTION}__. My stance is that * the removal of this indirection is so small that the current series can live without it---in fact, it does not benefit the current series at all---the only thing it brings us is a possible breakage for those we failed to consider their use case. * the removal of this indirection is so small but a future and unrelated usage.c improvement may benefit from it, so it should be justified within the context of that future series. It still may break the same folks whose use case we did not consider, but the other "improvement" in that future topic may offset the downside. So ... > ... since those functions won't deal well > with replacing a __FILE__ "just pass whatever data you'd like here". ... leave that as the justification for the _other_ series. It does not belong here in this series.
diff --git a/trace.h b/trace.h index 4e771f86ac2..5b52c1e23fd 100644 --- a/trace.h +++ b/trace.h @@ -126,19 +126,6 @@ void trace_command_performance(const char **argv); void trace_verbatim(struct trace_key *key, const void *buf, unsigned len); uint64_t trace_performance_enter(void); -/* - * 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. - */ -#ifndef TRACE_CONTEXT -# define TRACE_CONTEXT __FILE__ -#endif - /** * Macros to add the file:line of the calling code, instead of that of * the trace function itself. @@ -171,7 +158,7 @@ uint64_t trace_performance_enter(void); #define trace_printf_key(key, ...) \ do { \ if (trace_pass_fl(key)) \ - trace_printf_key_fl(TRACE_CONTEXT, __LINE__, key, \ + trace_printf_key_fl(__FILE__, __LINE__, key, \ __VA_ARGS__); \ } while (0) @@ -183,7 +170,7 @@ uint64_t trace_performance_enter(void); #define trace_argv_printf(argv, ...) \ do { \ if (trace_pass_fl(&trace_default_key)) \ - trace_argv_printf_fl(TRACE_CONTEXT, __LINE__, \ + trace_argv_printf_fl(__FILE__, __LINE__, \ argv, __VA_ARGS__); \ } while (0) @@ -196,7 +183,7 @@ uint64_t trace_performance_enter(void); #define trace_strbuf(key, data) \ do { \ if (trace_pass_fl(key)) \ - trace_strbuf_fl(TRACE_CONTEXT, __LINE__, key, data);\ + trace_strbuf_fl(__FILE__, __LINE__, key, data); \ } while (0) /** @@ -220,7 +207,7 @@ uint64_t trace_performance_enter(void); #define trace_performance(nanos, ...) \ do { \ if (trace_pass_fl(&trace_perf_key)) \ - trace_performance_fl(TRACE_CONTEXT, __LINE__, nanos,\ + trace_performance_fl(__FILE__, __LINE__, nanos, \ __VA_ARGS__); \ } while (0) @@ -239,7 +226,7 @@ uint64_t trace_performance_enter(void); #define trace_performance_since(start, ...) \ do { \ if (trace_pass_fl(&trace_perf_key)) \ - trace_performance_fl(TRACE_CONTEXT, __LINE__, \ + trace_performance_fl(__FILE__, __LINE__, \ getnanotime() - (start), \ __VA_ARGS__); \ } while (0) @@ -250,7 +237,7 @@ uint64_t trace_performance_enter(void); #define trace_performance_leave(...) \ do { \ if (trace_pass_fl(&trace_perf_key)) \ - trace_performance_leave_fl(TRACE_CONTEXT, __LINE__, \ + trace_performance_leave_fl(__FILE__, __LINE__, \ getnanotime(), \ __VA_ARGS__); \ } while (0)
The definition of "TRACE_CONTEXT" as "__FILE__" added in e05bed960d3 (trace: add 'file:line' to all trace output, 2014-07-12) appeared between v6[1] and v7[2] of the series that added it. It does not appear to have been something anybody asked for, and doesn't seem to have been used by anyone since then to override it to something other than the default __FILE__. When trace2 was added in ee4512ed481 (trace2: create new combined trace facility, 2019-02-22) it added no such "configuration", and we're unlikely to ever want to configure this through these means. I.e. I have considered (and probably will) make the usage.c macros support optional __FUNCTION__ at some point, that would need to have them passed as "mandatory" parameters (which might default to NULL) to the underlying function, for the reasons explained in the comment being removed here. So let's just remove this indirection in favor of using __FILE__ directly. 1. https://lore.kernel.org/git/53A4A2CD.8010003@gmail.com/ 2. https://lore.kernel.org/git/53B33DED.3030809@gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- trace.h | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-)