From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>, "Jeff King" <peff@peff.net>,
"brian m . carlson" <sandals@crustytoothpaste.net>,
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [PATCH v2 0/2] C99: remove hardcoded-out !HAVE_VARIADIC_MACROS code
Date: Fri, 28 Jan 2022 12:11:07 +0100 [thread overview]
Message-ID: <cover-v2-0.2-00000000000-20220128T110330Z-avarab@gmail.com> (raw)
In-Reply-To: <cover-0.2-00000000000-20210412T105422Z-avarab@gmail.com>
This v2 of $subject is being submitted on the one-year anniversary of
the patch to hardcode away the !HAVE_VARIADIC_MACROS code.
This is another small step on the way towards changing the usage.[ch]
API so that we can log the actual file/line number of
usage()/warning()/error()/die() in trace2 events. See a greater RFC
series to do that at:
https://lore.kernel.org/git/RFC-cover-00.21-00000000000-20211115T220831Z-avarab@gmail.com/
This should address all feedback raised on v1, including Junio
thinking it was a no-go since the submission in April 2021 was was
relatively recently after we'd hardcoded away
!HAVE_VARIADIC_MACROS. We've now had several releases with that code,
and other recent C99 test balloons since then.
The since of the range-diff is mainly because in v1 I didn't notice
that I needed to move the relevant API docs around, which this v2
does.
Ævar Arnfjörð Bjarmason (2):
git-compat-util.h: clarify GCC v.s. C99-specific in comment
C99: remove hardcoded-out !HAVE_VARIADIC_MACROS code
Documentation/CodingGuidelines | 3 +
banned.h | 5 --
git-compat-util.h | 16 +---
trace.c | 80 +-------------------
trace.h | 133 +++++++++++++--------------------
trace2.c | 39 ----------
trace2.h | 25 -------
usage.c | 15 +---
8 files changed, 59 insertions(+), 257 deletions(-)
Range-diff against v1:
1: a8cc05cf56f ! 1: 31079a71ecb git-compat-util.h: clarify comment on GCC-specific code
@@ Metadata
Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
## Commit message ##
- git-compat-util.h: clarify comment on GCC-specific code
+ git-compat-util.h: clarify GCC v.s. C99-specific in comment
Change a comment added in e208f9cc757 (make error()'s constant return
- value more visible, 2012-12-15) to note that the code doesn't only
- depend on variadic macros, which have been a hard dependency since
- 765dc168882 (git-compat-util: always enable variadic macros,
- 2021-01-28), but also on GCC's handling of __VA_ARGS__. The commit
- message for e208f9cc757 made this clear, but the comment it added did
- not.
+ value more visible, 2012-12-15). It's not correct that this is GCC-ism
+ anymore, it's code that uses standard C99 features.
+
+ The comment being changed here pre-dates the HAVE_VARIADIC_MACROS
+ define, which we got in e05bed960d3 (trace: add 'file:line' to all
+ trace output, 2014-07-12).
+
+ The original implementation of an error() macro) in e208f9cc757 used a
+ GCC-ism with the paste operator (see the commit message for mention of
+ it), but that was dropped later by 9798f7e5f9 (Use __VA_ARGS__ for all
+ of error's arguments, 2013-02-08), giving us the C99-portable version
+ we have now.
+
+ While we could remove the __GNUC__ define here, it might cause issues
+ for other compilers or static analysis systems, so let's not. See
+ 87fe5df365 (inline constant return from error() function, 2014-05-06)
+ for one such issue.
See also e05bed960d3 (trace: add 'file:line' to all trace output,
2014-07-12) for another comment about GNUC's handling of __VA_ARGS__.
@@ Commit message
## git-compat-util.h ##
@@ git-compat-util.h: void warning_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
-
/*
* Let callers be aware of the constant return value; this can help
-- * gcc with -Wuninitialized analysis. We restrict this trick to gcc, though,
+ * gcc with -Wuninitialized analysis. We restrict this trick to gcc, though,
- * because some compilers may not support variadic macros. Since we're only
- * trying to help gcc, anyway, it's OK; other compilers will fall back to
- * using the function as usual.
-+ * gcc with -Wuninitialized analysis.
-+ *
-+ * We restrict this trick to gcc, though, because while we rely on the
-+ * presence of C99 variadic macros, this code also relies on the
-+ * non-standard behavior of GCC's __VA_ARGS__, allowing error() to
-+ * work even if no format specifiers are passed to error().
-+ *
-+ * Since we're only trying to help gcc, anyway, it's OK; other
-+ * compilers will fall back to using the function as usual.
++ * because other compilers may be confused by this.
*/
#if defined(__GNUC__)
static inline int const_error(void)
2: f12e3cad57d ! 2: 966d96505cb C99 support: remove non-HAVE_VARIADIC_MACROS code
@@ Metadata
Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
## Commit message ##
- C99 support: remove non-HAVE_VARIADIC_MACROS code
+ C99: remove hardcoded-out !HAVE_VARIADIC_MACROS code
- Remove code that depend on HAVE_VARIADIC_MACROS not being set. Since
- 765dc168882 (git-compat-util: always enable variadic macros,
- 2021-01-28) we've unconditionally defined it to be true, and that
- change went out with v2.31.0. This should have given packagers enough
- time to discover whether variadic macros were an issue.
+ Remove the "else" branches of the HAVE_VARIADIC_MACROS macro, which
+ has been unconditionally omitted since 765dc168882 (git-compat-util:
+ always enable variadic macros, 2021-01-28).
- It seems that they weren't, so let's update the coding guidelines and
- remove all the fallback code for the non-HAVE_VARIADIC_MACROS case.
+ Since they were hardcoded out anyone trying to compile a version of
+ git v2.31.0 or later would have had a compilation error. 10 months
+ across a few releases since then should have been enough time for
+ anyone who cared to run into that and report the issue.
+
+ In addition to that, for anyone unsetting HAVE_VARIADIC_MACROS we've
+ been emitting extremely verbose warnings since at least
+ ee4512ed481 (trace2: create new combined trace facility,
+ 2019-02-22). That's because there is no such thing as a
+ "region_enter_printf" or "region_leave_printf" format, so at least
+ under GCC and Clang everything that includes trace.h (almost every
+ file) emits a couple of warnings about that.
+
+ There's a large benefit to being able to have a hard dependency rely
+ on variadic macros, the code surrounding usage.c is hard to maintain
+ if we need to write two implementations of everything, and by relying
+ on "__FILE__" and "__LINE__" along with "__VA_ARGS__" we can in the
+ future make error(), die() etc. log where they were called from. We've
+ also recently merged d67fc4bf0ba (Merge branch 'bc/require-c99',
+ 2021-12-10) which further cements our hard dependency on C99.
+
+ So let's delete the fallback code, and update our CodingGuidelines to
+ note that we depend on this. The added bullet-point starts with
+ lower-case for consistency with other bullet-points in that section.
+
+ The diff in "trace.h" is relatively hard to read, since we need to
+ retain the existing API docs, which were comments on the code used if
+ HAVE_VARIADIC_MACROS was not defined.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
@@ git-compat-util.h: static inline int regexec_buf(const regex_t *preg, const char
## trace.c ##
@@ trace.c: static int prepare_trace_line(const char *file, int line,
- strbuf_addf(buf, "%02d:%02d:%02d.%06ld ", tm.tm_hour, tm.tm_min,
- tm.tm_sec, (long) tv.tv_usec);
-
+ gettimeofday(&tv, NULL);
+ secs = tv.tv_sec;
+ localtime_r(&secs, &tm);
+- strbuf_addf(buf, "%02d:%02d:%02d.%06ld ", tm.tm_hour, tm.tm_min,
+- tm.tm_sec, (long) tv.tv_usec);
+-
-#ifdef HAVE_VARIADIC_MACROS
- /* print file:line */
- strbuf_addf(buf, "%s:%d ", file, line);
+- /* print file:line */
+- strbuf_addf(buf, "%s:%d ", file, line);
++ strbuf_addf(buf, "%02d:%02d:%02d.%06ld %s:%d", tm.tm_hour, tm.tm_min,
++ tm.tm_sec, (long) tv.tv_usec, file, line);
/* align trace output (column 40 catches most files names in git) */
while (buf->len < 40)
strbuf_addch(buf, ' ');
@@ trace.h: void trace_command_performance(const char **argv);
- */
-void trace_strbuf(struct trace_key *key, const struct strbuf *data);
-
--/**
+ /**
- * Prints elapsed time (in nanoseconds) if GIT_TRACE_PERFORMANCE is enabled.
-- *
++ * Macros to add the file:line of the calling code, instead of that of
++ * the trace function itself.
+ *
- * Example:
- * ------------
- * uint64_t t = 0;
@@ trace.h: void trace_command_performance(const char **argv);
-
-#else
-
- /*
- * Macros to add file:line - see above for C-style declarations of how these
- * should be used.
+-/*
+- * Macros to add file:line - see above for C-style declarations of how these
+- * should be used.
+- */
+-
+-/*
+- * 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
+-
+-/*
+ * Note: with C99 variadic macros, __VA_ARGS__ must include the last fixed
+ * parameter ('format' in this case). Otherwise, a call without variable
+ * arguments will have a surplus ','. E.g.:
+@@ trace.h: void trace_performance_leave(const char *format, ...);
+ *
+ * which is invalid (note the ',)'). With GNUC, '##__VA_ARGS__' drops the
+ * comma, but this is non-standard.
++ *
++ * 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
++
++/**
++ * Prints a formatted message, similar to printf.
+ */
++#define trace_printf(...) trace_printf_key(&trace_default_key, __VA_ARGS__)
+
+ #define trace_printf_key(key, ...) \
+ do { \
+@@ trace.h: void trace_performance_leave(const char *format, ...);
+ __VA_ARGS__); \
+ } while (0)
+
+-#define trace_printf(...) trace_printf_key(&trace_default_key, __VA_ARGS__)
+-
++/**
++ * Prints a formatted message, followed by a quoted list of arguments.
++ */
+ #define trace_argv_printf(argv, ...) \
+ do { \
+ if (trace_pass_fl(&trace_default_key)) \
+@@ trace.h: void trace_performance_leave(const char *format, ...);
+ argv, __VA_ARGS__); \
+ } while (0)
+
++/**
++ * Prints the strbuf, without additional formatting (i.e. doesn't
++ * choke on `%` or even `\0`).
++ */
+ #define trace_strbuf(key, data) \
+ do { \
+ if (trace_pass_fl(key)) \
+ trace_strbuf_fl(TRACE_CONTEXT, __LINE__, key, data);\
+ } while (0)
+
++/**
++ * Prints elapsed time (in nanoseconds) if GIT_TRACE_PERFORMANCE is enabled.
++ *
++ * Example:
++ * ------------
++ * uint64_t t = 0;
++ * for (;;) {
++ * // ignore
++ * t -= getnanotime();
++ * // code section to measure
++ * t += getnanotime();
++ * // ignore
++ * }
++ * trace_performance(t, "frotz");
++ * ------------
++ */
+ #define trace_performance(nanos, ...) \
+ do { \
+ if (trace_pass_fl(&trace_perf_key)) \
+@@ trace.h: void trace_performance_leave(const char *format, ...);
+ __VA_ARGS__); \
+ } while (0)
+
++/**
++ * Prints elapsed time since 'start' if GIT_TRACE_PERFORMANCE is enabled.
++ *
++ * Example:
++ * ------------
++ * uint64_t start = getnanotime();
++ * // code section to measure
++ * trace_performance_since(start, "foobar");
++ * ------------
++ */
+ #define trace_performance_since(start, ...) \
+ do { \
+ if (trace_pass_fl(&trace_perf_key)) \
+@@ trace.h: void trace_performance_leave(const char *format, ...);
+ __VA_ARGS__); \
+ } while (0)
+
++
+ /* backend functions, use non-*fl macros instead */
+ __attribute__((format (printf, 4, 5)))
+ void trace_printf_key_fl(const char *file, int line, struct trace_key *key,
@@ trace.h: static inline int trace_pass_fl(struct trace_key *key)
return key->fd || !key->initialized;
}
@@ trace2.h: void trace2_printf_va_fl(const char *file, int line, const char *fmt,
* Optional platform-specific code to dump information about the
## usage.c ##
+@@ usage.c: static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, va_lis
+ va_copy(params_copy, params);
+
+ /* truncation via snprintf is OK here */
+- if (file)
+- snprintf(prefix, sizeof(prefix), "BUG: %s:%d: ", file, line);
+- else
+- snprintf(prefix, sizeof(prefix), "BUG: ");
++ snprintf(prefix, sizeof(prefix), "BUG: %s:%d: ", file, line);
+
+ vreportf(prefix, fmt, params);
+
@@ usage.c: static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, va_lis
abort();
}
--
2.35.0.912.ga4a35ddedc3
next prev parent reply other threads:[~2022-01-28 11:11 UTC|newest]
Thread overview: 71+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-10 15:21 [PATCH 00/15] SHA-256 / SHA-1 interop, part 1 brian m. carlson
2021-04-10 15:21 ` [PATCH 01/15] sha1-file: allow hashing objects literally with any algorithm brian m. carlson
2021-04-15 8:55 ` Denton Liu
2021-04-15 23:03 ` brian m. carlson
2021-04-16 15:04 ` Ævar Arnfjörð Bjarmason
2021-04-16 18:55 ` Junio C Hamano
2021-04-10 15:21 ` [PATCH 02/15] builtin/hash-object: allow literally hashing with a given algorithm brian m. carlson
2021-04-11 8:52 ` Ævar Arnfjörð Bjarmason
2021-04-11 21:07 ` brian m. carlson
2021-04-16 15:21 ` Ævar Arnfjörð Bjarmason
2021-04-16 17:27 ` Ævar Arnfjörð Bjarmason
2021-04-10 15:21 ` [PATCH 03/15] cache: add an algo member to struct object_id brian m. carlson
2021-04-11 11:55 ` Ævar Arnfjörð Bjarmason
2021-04-11 21:37 ` brian m. carlson
2021-04-13 12:12 ` Derrick Stolee
2021-04-14 1:08 ` brian m. carlson
2021-04-15 8:47 ` Ævar Arnfjörð Bjarmason
2021-04-15 23:51 ` brian m. carlson
2021-04-10 15:21 ` [PATCH 04/15] Always use oidread to read into " brian m. carlson
2021-04-10 15:21 ` [PATCH 05/15] hash: add a function to finalize object IDs brian m. carlson
2021-04-10 15:21 ` [PATCH 06/15] Use the final_oid_fn to finalize hashing of " brian m. carlson
2021-04-10 15:21 ` [PATCH 07/15] builtin/pack-redundant: avoid casting buffers to struct object_id brian m. carlson
2021-04-10 15:21 ` [PATCH 08/15] cache: compare the entire buffer for " brian m. carlson
2021-04-11 8:17 ` Chris Torek
2021-04-11 11:36 ` Ævar Arnfjörð Bjarmason
2021-04-11 21:05 ` brian m. carlson
2021-04-10 15:21 ` [PATCH 09/15] hash: set and copy algo field in " brian m. carlson
2021-04-11 11:57 ` Ævar Arnfjörð Bjarmason
2021-04-11 21:48 ` brian m. carlson
2021-04-11 22:12 ` Ævar Arnfjörð Bjarmason
2021-04-11 23:52 ` brian m. carlson
2021-04-12 11:02 ` [PATCH 0/2] C99: harder dependency on variadic macros Ævar Arnfjörð Bjarmason
2021-04-12 11:02 ` [PATCH 1/2] git-compat-util.h: clarify comment on GCC-specific code Ævar Arnfjörð Bjarmason
2021-04-13 7:57 ` Jeff King
2021-04-13 21:07 ` Junio C Hamano
2021-04-14 5:21 ` Jeff King
2021-04-14 6:12 ` Ævar Arnfjörð Bjarmason
2021-04-14 7:31 ` Jeff King
2021-05-21 2:06 ` Jonathan Nieder
2021-04-12 11:02 ` [PATCH 2/2] C99 support: remove non-HAVE_VARIADIC_MACROS code Ævar Arnfjörð Bjarmason
2021-04-12 17:58 ` Junio C Hamano
2021-04-13 8:00 ` Jeff King
2021-05-21 2:50 ` Jonathan Nieder
2021-04-12 12:14 ` [PATCH 0/2] C99: harder dependency on variadic macros Bagas Sanjaya
2021-04-12 12:41 ` Ævar Arnfjörð Bjarmason
2021-04-12 22:57 ` brian m. carlson
2021-04-12 23:19 ` Junio C Hamano
2022-01-28 11:11 ` Ævar Arnfjörð Bjarmason [this message]
2022-01-28 11:11 ` [PATCH v2 1/2] git-compat-util.h: clarify GCC v.s. C99-specific in comment Ævar Arnfjörð Bjarmason
2022-01-28 11:11 ` [PATCH v2 2/2] C99: remove hardcoded-out !HAVE_VARIADIC_MACROS code Ævar Arnfjörð Bjarmason
2022-01-28 22:40 ` Junio C Hamano
2022-02-19 10:41 ` [PATCH v3 0/3] C99: remove dead " Ævar Arnfjörð Bjarmason
2022-02-19 10:41 ` [PATCH v3 1/3] git-compat-util.h: clarify GCC v.s. C99-specific in comment Ævar Arnfjörð Bjarmason
2022-02-19 10:41 ` [PATCH v3 2/3] C99: remove hardcoded-out !HAVE_VARIADIC_MACROS code Ævar Arnfjörð Bjarmason
2022-02-19 10:41 ` [PATCH v3 3/3] trace.h: remove never-used TRACE_CONTEXT Ævar Arnfjörð Bjarmason
2022-02-20 12:02 ` Junio C Hamano
2022-02-20 12:38 ` Ævar Arnfjörð Bjarmason
2022-02-20 20:12 ` Junio C Hamano
2022-02-21 16:05 ` [PATCH v4 0/2] C99: remove dead !HAVE_VARIADIC_MACROS code Ævar Arnfjörð Bjarmason
2022-02-21 16:05 ` [PATCH v4 1/2] git-compat-util.h: clarify GCC v.s. C99-specific in comment Ævar Arnfjörð Bjarmason
2022-02-21 16:05 ` [PATCH v4 2/2] C99: remove hardcoded-out !HAVE_VARIADIC_MACROS code Ævar Arnfjörð Bjarmason
2021-04-12 10:53 ` [PATCH 09/15] hash: set and copy algo field in struct object_id Junio C Hamano
2021-04-12 11:13 ` Ævar Arnfjörð Bjarmason
2021-04-10 15:21 ` [PATCH 10/15] hash: provide per-algorithm null OIDs brian m. carlson
2021-04-11 14:03 ` Junio C Hamano
2021-04-11 21:51 ` brian m. carlson
2021-04-10 15:21 ` [PATCH 11/15] builtin/show-index: set the algorithm for object IDs brian m. carlson
2021-04-10 15:21 ` [PATCH 12/15] commit-graph: don't store file hashes as struct object_id brian m. carlson
2021-04-10 15:21 ` [PATCH 13/15] builtin/pack-objects: avoid using struct object_id for pack hash brian m. carlson
2021-04-10 15:21 ` [PATCH 14/15] hex: default to the_hash_algo on zero algorithm value brian m. carlson
2021-04-10 15:21 ` [PATCH 15/15] hex: print objects using the hash algorithm member brian m. carlson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cover-v2-0.2-00000000000-20220128T110330Z-avarab@gmail.com \
--to=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=peff@peff.net \
--cc=sandals@crustytoothpaste.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).