git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Matt Kraai <kraai@ftbfs.org>
Cc: git@vger.kernel.org, Max Horn <max@quendi.de>,
	Junio C Hamano <gitster@pobox.com>,
	Matt Kraai <matt.kraai@amo.abbott.com>
Subject: Re: [PATCH] Use __VA_ARGS__ for all of error's arguments
Date: Thu, 7 Feb 2013 23:24:28 -0500	[thread overview]
Message-ID: <20130208042428.GA4157@sigill.intra.peff.net> (raw)
In-Reply-To: <1360272632-22566-1-git-send-email-kraai@ftbfs.org>

On Thu, Feb 07, 2013 at 01:30:32PM -0800, Matt Kraai wrote:

> From: Matt Kraai <matt.kraai@amo.abbott.com>
> 
> QNX 6.3.2 uses GCC 2.95.3 by default, and GCC 2.95.3 doesn't remove the
> comma if the error macro's variable argument is left out.
> 
> Instead of testing for a sufficiently recent version of GCC, make
> __VA_ARGS__ match all of the arguments.

Thanks, this looks better than the original (we do not assume a C99
compiler, so just doing this unconditionally would probably break some
other older systems which do not use gcc).

>  /*
>   * Let callers be aware of the constant return value; this can help
> - * gcc with -Wuninitialized analysis. We have to restrict this trick to
> - * gcc, though, because of the variadic macro and the magic ## comma pasting
> - * behavior. But 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.
>   */
>  #if defined(__GNUC__) && ! defined(__clang__)
> -#define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)
> +#define error(...) (error(__VA_ARGS__), -1)

Should you be dropping most of the comment like this? I would expect it
to be more like:

  We have to restrict this trick to gcc, though, because we do not
  assume all compilers support variadic macros. But since...

Other than that, I think it is OK. The compiler will still catch
"error()" with no arguments and generate the appropriate diagnostic (in
fact, it is better, because the error is now passing too few args to a
function, not to the macro).

-Peff

  reply	other threads:[~2013-02-08  4:24 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-02-07 20:00 [PATCH] Use __VA_ARGS__ for all of error's arguments Matt Kraai
2013-02-07 21:05 ` Junio C Hamano
2013-02-07 21:14   ` John Keeping
2013-02-07 21:24   ` Matt Kraai
2013-02-07 21:30     ` Matt Kraai
2013-02-08  4:24       ` Jeff King [this message]
2013-02-08  4:39         ` Matt Kraai
2013-02-08 15:09           ` Matt Kraai
2013-02-08 15:54             ` Jeff King

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

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20130208042428.GA4157@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=kraai@ftbfs.org \
    --cc=matt.kraai@amo.abbott.com \
    --cc=max@quendi.de \
    /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.
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 NNTP newsgroup(s).