diff mbox series

[1/2] git-compat-util.h: clarify comment on GCC-specific code

Message ID patch-1.2-a8cc05cf56f-20210412T105422Z-avarab@gmail.com (mailing list archive)
State New
Headers show
Series C99: harder dependency on variadic macros | expand

Commit Message

Ævar Arnfjörð Bjarmason April 12, 2021, 11:02 a.m. UTC
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(-)

Comments

Jeff King April 13, 2021, 7:57 a.m. UTC | #1
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
Junio C Hamano April 13, 2021, 9:07 p.m. UTC | #2
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.
Jeff King April 14, 2021, 5:21 a.m. UTC | #3
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
Ævar Arnfjörð Bjarmason April 14, 2021, 6:12 a.m. UTC | #4
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?
Jeff King April 14, 2021, 7:31 a.m. UTC | #5
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
diff mbox series

Patch

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)