| Message ID | patch-1.2-a8cc05cf56f-20210412T105422Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | C99: harder dependency on variadic macros | expand |
On Mon, Apr 12, 2021 at 01:02:17PM +0200, Ævar Arnfjörð Bjarmason wrote: > diff --git a/git-compat-util.h b/git-compat-util.h > index 9ddf9d7044b..540aba22a4d 100644 > --- a/git-compat-util.h > +++ b/git-compat-util.h > @@ -480,10 +480,15 @@ 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, > - * 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. > */ > #if defined(__GNUC__) I don't mind leaving this gcc-only, since as you note that's the point of what the code is trying to do. But wouldn't this always work because we know there is at least one arg (the format itself)? I.e., if we had written: #define error(fmt, ...) (error(fmt, __VA_ARGS__), const_error()) that would be a problem for: error("foo"); But because we wrote: #define error(...) (error(__VA_ARGS__), const_error()) then it's OK. -Peff
Jeff King <peff@peff.net> writes: >> + * 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(). The last part of this comment is puzzlling. Do we ever call error() without any format specifier? There may be GCC-ism behaviour around the __VA_ARGS__ stuff, but are we relying on that GCC-ism? >> + * Since we're only trying to help gcc, anyway, it's OK; other >> + * compilers will fall back to using the function as usual. >> */ >> #if defined(__GNUC__) > > I don't mind leaving this gcc-only, since as you note that's the point > of what the code is trying to do. But wouldn't this always work because > we know there is at least one arg (the format itself)? > > I.e., if we had written: > > #define error(fmt, ...) (error(fmt, __VA_ARGS__), const_error()) > > that would be a problem for: > > error("foo"); > > But because we wrote: > > #define error(...) (error(__VA_ARGS__), const_error()) > > then it's OK. I think so. At least I find the new comment confusing, and I'd prefer to see it cleaned up. Thanks.
On Tue, Apr 13, 2021 at 02:07:13PM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > >> + * 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(). > > The last part of this comment is puzzlling. Do we ever call error() > without any format specifier? There may be GCC-ism behaviour around > the __VA_ARGS__ stuff, but are we relying on that GCC-ism? I took "format specifier" to mean the "%" code within the format. E.g.: error("foo"); has no format specifier, and thus no arguments after the format. But every call will have at least the format string itself. AFAIK, portably using variadic macros means you need there to always be at least one argument. Hence "error(fmt, ...)" is wrong (the "..." may have no arguments) but "error(...)" is OK (you always have a format string). I'm not sure if Ævar knows about some other portability gotcha, or if he just didn't realize that this was written in the portable way. -Peff
On Wed, Apr 14 2021, Jeff King wrote: > On Tue, Apr 13, 2021 at 02:07:13PM -0700, Junio C Hamano wrote: > >> Jeff King <peff@peff.net> writes: >> >> >> + * 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(). >> >> The last part of this comment is puzzlling. Do we ever call error() >> without any format specifier? There may be GCC-ism behaviour around >> the __VA_ARGS__ stuff, but are we relying on that GCC-ism? > > I took "format specifier" to mean the "%" code within the format. E.g.: > > error("foo"); > > has no format specifier, and thus no arguments after the format. But > every call will have at least the format string itself. > > AFAIK, portably using variadic macros means you need there to always be > at least one argument. Hence "error(fmt, ...)" is wrong (the "..." may > have no arguments) but "error(...)" is OK (you always have a format > string). I'm not sure if Ævar knows about some other portability gotcha, > or if he just didn't realize that this was written in the portable way. No, I just read elsewhere that GCC had non-standard behavior, and didn't look carefully at your implementation, but since it explicitly depended on GNUC etc. understood it to mean it was GCC-specific, not just C99-specific. So it can simply be changed to depend on HAVE_VARIADIC_MACROS instead?
On Wed, Apr 14, 2021 at 08:12:34AM +0200, Ævar Arnfjörð Bjarmason wrote: > > On Wed, Apr 14 2021, Jeff King wrote: > > > On Tue, Apr 13, 2021 at 02:07:13PM -0700, Junio C Hamano wrote: > > > >> Jeff King <peff@peff.net> writes: > >> > >> >> + * 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(). > >> > >> The last part of this comment is puzzlling. Do we ever call error() > >> without any format specifier? There may be GCC-ism behaviour around > >> the __VA_ARGS__ stuff, but are we relying on that GCC-ism? > > > > I took "format specifier" to mean the "%" code within the format. E.g.: > > > > error("foo"); > > > > has no format specifier, and thus no arguments after the format. But > > every call will have at least the format string itself. > > > > AFAIK, portably using variadic macros means you need there to always be > > at least one argument. Hence "error(fmt, ...)" is wrong (the "..." may > > have no arguments) but "error(...)" is OK (you always have a format > > string). I'm not sure if Ævar knows about some other portability gotcha, > > or if he just didn't realize that this was written in the portable way. > > No, I just read elsewhere that GCC had non-standard behavior, and didn't > look carefully at your implementation, but since it explicitly depended > on GNUC etc. understood it to mean it was GCC-specific, not just > C99-specific. > > So it can simply be changed to depend on HAVE_VARIADIC_MACROS instead? I think it probably could be, yes. The original predates HAVE_VARIADIC_MACROS (which we got in 2014); the original error() macro is from e208f9cc75 (make error()'s constant return value more visible, 2012-12-15). The original also used the gcc-ism with the paste operator (see the commit message for mention of it), but that was actually 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. All that said, I don't see much value in converting it to use HAVE_VARIADIC_MACROS. It is mostly there to benefit gcc's warning code. In theory making the return value visible could help other compilers generate better code, too. If we care about doing that, we could switch to making it unconditional. But aside from the variadic macros, it's kind of a weird construct and I'd be worried about generating warnings on other compilers or static analysis systems. E.g., see the hack from 87fe5df365 (inline constant return from error() function, 2014-05-06). -Peff
Hi, Ævar Arnfjörð Bjarmason wrote: > 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. > > See also e05bed960d3 (trace: add 'file:line' to all trace output, > 2014-07-12) for another comment about GNUC's handling of __VA_ARGS__. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > git-compat-util.h | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) Oh, subtle. In fact, I believe 9798f7e5f9 (Use __VA_ARGS__ for all of error's arguments, 2013-02-08) got rid of the gcc-ism, so we could do the following instead. (See section 6.10.3.10 for a description of #define f(...) g(__VA_ARGS__) in the C99 standard.) Thanks, Jonathan -- >8 -- Subject: error: use macro-based static analysis aid on non-gcc, too In the rest of Git, we check HAVE_VARIADIC_MACROS (which is unconditionally defined to true as a way to discover platforms that do not support it) to guard code that requires variadic macro support. This variadic macro is a bit older, so it uses a __GNUC__ check instead. Switch to use of HAVE_VARIADIC_MACROS here as well, so more compilers can get the benefit of knowing at compile time that error() always returns -1. Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> --- git-compat-util.h | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/git-compat-util.h b/git-compat-util.h index a508dbe5a3..ca22b11760 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -483,14 +483,19 @@ void warning_errno(const char *err, ...) __attribute__((format (printf, 1, 2))); #include <openssl/x509v3.h> #endif /* NO_OPENSSL */ +/* + * This is always defined as a first step towards making the use of variadic + * macros unconditional. If it causes compilation problems on your platform, + * please report it to the Git mailing list at git@vger.kernel.org. + */ +#define HAVE_VARIADIC_MACROS 1 + /* * Let callers be aware of the constant return value; this can help - * 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. Compilers without support for variadic + * macros will fall back to using the function as usual. */ -#if defined(__GNUC__) +#ifdef HAVE_VARIADIC_MACROS static inline int const_error(void) { return -1; @@ -1192,13 +1197,6 @@ static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size, #endif #endif -/* - * This is always defined as a first step towards making the use of variadic - * macros unconditional. If it causes compilation problems on your platform, - * please report it to the Git mailing list at git@vger.kernel.org. - */ -#define HAVE_VARIADIC_MACROS 1 - /* usage.c: only to be used for testing BUG() implementation (see test-tool) */ extern int BUG_exit_code;
diff --git a/git-compat-util.h b/git-compat-util.h index 9ddf9d7044b..540aba22a4d 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -480,10 +480,15 @@ 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, - * 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. */ #if defined(__GNUC__) static inline int const_error(void)
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. See also e05bed960d3 (trace: add 'file:line' to all trace output, 2014-07-12) for another comment about GNUC's handling of __VA_ARGS__. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- git-compat-util.h | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)