{"thread":{"id":"32851","subject":"[PATCH] Use __VA_ARGS__ for all of error's arguments","startedAt":"2013-02-07T20:00:38Z","lastAt":"2013-02-08T15:54:16Z","messageCount":9,"participants":["Matt Kraai","Junio C Hamano","John Keeping","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"208941","messageId":"1360267238-21896-1-git-send-email-kraai@ftbfs.org","threadId":"32851","inReplyTo":null,"subject":"[PATCH] Use __VA_ARGS__ for all of error's arguments","fromName":"Matt Kraai","fromEmail":"kraai@ftbfs.org","sentAt":"2013-02-07T20:00:38Z","receivedAt":"2013-02-07T20:00:38Z","isPatch":true,"sender":{"key":"kraai@ftbfs.org","avatar":null},"body":"From: Matt Kraai \n\nQNX 6.3.2 uses GCC 2.95.3 by default, and GCC 2.95.3 doesn't remove the\ncomma if the error macro's variable argument is left out.\n\nInstead of testing for a sufficiently recent version of GCC, make\n__VA_ARGS__ match all of the arguments. Since this should work on any\nC99-compliant compiler, also remove the tests around the macro definition.\n\nSigned-off-by: Matt Kraai \n---\n git-compat-util.h | 9 ++-------\n 1 file changed, 2 insertions(+), 7 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex cc2abee..df1681f 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -305,14 +305,9 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)))\n \n /*\n * Let callers be aware of the constant return value; this can help\n- * gcc with -Wuninitialized analysis. We have to restrict this trick to\n- * gcc, though, because of the variadic macro and the magic ## comma pasting\n- * behavior. But since we're only trying to help gcc, anyway, it's OK; other\n- * compilers will fall back to using the function as usual.\n+ * gcc with -Wuninitialized analysis.\n */\n-#if defined(__GNUC__) && ! defined(__clang__)\n-#define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)\n-#endif\n+#define error(...) (error(__VA_ARGS__), -1)\n \n extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));\n extern void set_error_routine(void (*routine)(const char *err, va_list params));\n-- \n1.8.1.GIT\n"},{"id":"208951","messageId":"7vwquj6dio.fsf@alter.siamese.dyndns.org","threadId":"32851","inReplyTo":"1360267238-21896-1-git-send-email-kraai@ftbfs.org","subject":"Re: [PATCH] Use __VA_ARGS__ for all of error's arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-07T21:05:19Z","receivedAt":"2013-02-07T21:05:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matt Kraai writes:\n\n> -#if defined(__GNUC__) && ! defined(__clang__)\n> -#define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)\n> -#endif\n> +#define error(...) (error(__VA_ARGS__), -1)\n\nBefore your change, we only define error() macro for GCC variants,\nbut with your patch that no longer is the case. Does every compiler\nthat compiles Git correctly today support this style of varargs\nmacros?\n"},{"id":"208952","messageId":"20130207211449.GC1342@serenity.lan","threadId":"32851","inReplyTo":"7vwquj6dio.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Use __VA_ARGS__ for all of error's arguments","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-02-07T21:14:49Z","receivedAt":"2013-02-07T21:14:49Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Thu, Feb 07, 2013 at 01:05:19PM -0800, Junio C Hamano wrote:\n> Matt Kraai writes:\n> \n> > -#if defined(__GNUC__) && ! defined(__clang__)\n> > -#define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)\n> > -#endif\n> > +#define error(...) (error(__VA_ARGS__), -1)\n> \n> Before your change, we only define error() macro for GCC variants,\n> but with your patch that no longer is the case. Does every compiler\n> that compiles Git correctly today support this style of varargs\n> macros?\n\nAt the very least the \"! defined(__clang__)\" was only recently added\n[1], although it shouldn't be needed once Clang 3.3 is released.\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/213787\n\n\nJohn\n"},{"id":"208953","messageId":"20130207212438.GA22253@ftbfs.org","threadId":"32851","inReplyTo":"7vwquj6dio.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Use __VA_ARGS__ for all of error's arguments","fromName":"Matt Kraai","fromEmail":"kraai@ftbfs.org","sentAt":"2013-02-07T21:24:38Z","receivedAt":"2013-02-07T21:24:38Z","isPatch":true,"sender":{"key":"kraai@ftbfs.org","avatar":null},"body":"On Thu, Feb 07, 2013 at 01:05:19PM -0800, Junio C Hamano wrote:\n> Matt Kraai writes:\n> \n> > -#if defined(__GNUC__) && ! defined(__clang__)\n> > -#define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)\n> > -#endif\n> > +#define error(...) (error(__VA_ARGS__), -1)\n> \n> Before your change, we only define error() macro for GCC variants,\n> but with your patch that no longer is the case. Does every compiler\n> that compiles Git correctly today support this style of varargs\n> macros?\n\nI don't know and I don't think it's likely I can confirm this. I'll\nsubmit a new patch that just changes the definition, since I don't\nknow of any problems other than mine with the current situation.\n\n-- \nMatt\n"},{"id":"208954","messageId":"1360272632-22566-1-git-send-email-kraai@ftbfs.org","threadId":"32851","inReplyTo":"20130207212438.GA22253@ftbfs.org","subject":"[PATCH] Use __VA_ARGS__ for all of error's arguments","fromName":"Matt Kraai","fromEmail":"kraai@ftbfs.org","sentAt":"2013-02-07T21:30:32Z","receivedAt":"2013-02-07T21:30:32Z","isPatch":true,"sender":{"key":"kraai@ftbfs.org","avatar":null},"body":"From: Matt Kraai \n\nQNX 6.3.2 uses GCC 2.95.3 by default, and GCC 2.95.3 doesn't remove the\ncomma if the error macro's variable argument is left out.\n\nInstead of testing for a sufficiently recent version of GCC, make\n__VA_ARGS__ match all of the arguments.\n\nSigned-off-by: Matt Kraai \n---\n git-compat-util.h | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex cc2abee..2e960a9 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -305,13 +305,10 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)))\n \n /*\n * Let callers be aware of the constant return value; this can help\n- * gcc with -Wuninitialized analysis. We have to restrict this trick to\n- * gcc, though, because of the variadic macro and the magic ## comma pasting\n- * behavior. But since we're only trying to help gcc, anyway, it's OK; other\n- * compilers will fall back to using the function as usual.\n+ * gcc with -Wuninitialized analysis.\n */\n #if defined(__GNUC__) && ! defined(__clang__)\n-#define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)\n+#define error(...) (error(__VA_ARGS__), -1)\n #endif\n \n extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));\n-- \n1.8.1.2.546.gfc9f004\n"},{"id":"208969","messageId":"20130208042428.GA4157@sigill.intra.peff.net","threadId":"32851","inReplyTo":"1360272632-22566-1-git-send-email-kraai@ftbfs.org","subject":"Re: [PATCH] Use __VA_ARGS__ for all of error's arguments","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-08T04:24:28Z","receivedAt":"2013-02-08T04:24:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 07, 2013 at 01:30:32PM -0800, Matt Kraai wrote:\n\n> From: Matt Kraai \n> \n> QNX 6.3.2 uses GCC 2.95.3 by default, and GCC 2.95.3 doesn't remove the\n> comma if the error macro's variable argument is left out.\n> \n> Instead of testing for a sufficiently recent version of GCC, make\n> __VA_ARGS__ match all of the arguments.\n\nThanks, this looks better than the original (we do not assume a C99\ncompiler, so just doing this unconditionally would probably break some\nother older systems which do not use gcc).\n\n> /*\n> * Let callers be aware of the constant return value; this can help\n> - * gcc with -Wuninitialized analysis. We have to restrict this trick to\n> - * gcc, though, because of the variadic macro and the magic ## comma pasting\n> - * behavior. But since we're only trying to help gcc, anyway, it's OK; other\n> - * compilers will fall back to using the function as usual.\n> + * gcc with -Wuninitialized analysis.\n> */\n> #if defined(__GNUC__) && ! defined(__clang__)\n> -#define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)\n> +#define error(...) (error(__VA_ARGS__), -1)\n\nShould you be dropping most of the comment like this? I would expect it\nto be more like:\n\n We have to restrict this trick to gcc, though, because we do not\n assume all compilers support variadic macros. But since...\n\nOther than that, I think it is OK. The compiler will still catch\n\"error()\" with no arguments and generate the appropriate diagnostic (in\nfact, it is better, because the error is now passing too few args to a\nfunction, not to the macro).\n\n-Peff\n"},{"id":"208971","messageId":"20130208043915.GB4525@ftbfs.org","threadId":"32851","inReplyTo":"20130208042428.GA4157@sigill.intra.peff.net","subject":"Re: [PATCH] Use __VA_ARGS__ for all of error's arguments","fromName":"Matt Kraai","fromEmail":"kraai@ftbfs.org","sentAt":"2013-02-08T04:39:16Z","receivedAt":"2013-02-08T04:39:16Z","isPatch":true,"sender":{"key":"kraai@ftbfs.org","avatar":null},"body":"On Thu, Feb 07, 2013 at 11:24:28PM -0500, Jeff King wrote:\n> Should you be dropping most of the comment like this? I would expect it\n> to be more like:\n> \n> We have to restrict this trick to gcc, though, because we do not\n> assume all compilers support variadic macros. But since...\n\nI'll submit a new patch with this change tomorrow.\n\n> Other than that, I think it is OK. The compiler will still catch\n> \"error()\" with no arguments and generate the appropriate diagnostic (in\n> fact, it is better, because the error is now passing too few args to a\n> function, not to the macro).\n\nGreat, thanks for the review.\n\n-- \nMatt\n"},{"id":"208994","messageId":"1360336168-27740-1-git-send-email-kraai@ftbfs.org","threadId":"32851","inReplyTo":"20130208043915.GB4525@ftbfs.org","subject":"[PATCH] Use __VA_ARGS__ for all of error's arguments","fromName":"Matt Kraai","fromEmail":"kraai@ftbfs.org","sentAt":"2013-02-08T15:09:28Z","receivedAt":"2013-02-08T15:09:28Z","isPatch":true,"sender":{"key":"kraai@ftbfs.org","avatar":null},"body":"From: Matt Kraai \n\nQNX 6.3.2 uses GCC 2.95.3 by default, and GCC 2.95.3 doesn't remove the\ncomma if the error macro's variable argument is left out.\n\nInstead of testing for a sufficiently recent version of GCC, make\n__VA_ARGS__ match all of the arguments.\n\nSigned-off-by: Matt Kraai \n---\n git-compat-util.h | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex cc2abee..b7eaaa9 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -305,13 +305,13 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)))\n \n /*\n * Let callers be aware of the constant return value; this can help\n- * gcc with -Wuninitialized analysis. We have to restrict this trick to\n- * gcc, though, because of the variadic macro and the magic ## comma pasting\n- * behavior. But since we're only trying to help gcc, anyway, it's OK; other\n- * compilers will fall back to using the function as usual.\n+ * gcc with -Wuninitialized analysis. We restrict this trick to gcc, though,\n+ * because some compilers may not support variadic macros. Since we're only\n+ * trying to help gcc, anyway, it's OK; other compilers will fall back to\n+ * using the function as usual.\n */\n #if defined(__GNUC__) && ! defined(__clang__)\n-#define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)\n+#define error(...) (error(__VA_ARGS__), -1)\n #endif\n \n extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));\n-- \n1.8.1.2.546.g90a97a4\n"},{"id":"208997","messageId":"20130208155416.GA20874@sigill.intra.peff.net","threadId":"32851","inReplyTo":"1360336168-27740-1-git-send-email-kraai@ftbfs.org","subject":"Re: [PATCH] Use __VA_ARGS__ for all of error's arguments","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-08T15:54:16Z","receivedAt":"2013-02-08T15:54:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 08, 2013 at 07:09:28AM -0800, Matt Kraai wrote:\n\n> From: Matt Kraai \n> \n> QNX 6.3.2 uses GCC 2.95.3 by default, and GCC 2.95.3 doesn't remove the\n> comma if the error macro's variable argument is left out.\n> \n> Instead of testing for a sufficiently recent version of GCC, make\n> __VA_ARGS__ match all of the arguments.\n>\n> [...]\n>\n> /*\n> * Let callers be aware of the constant return value; this can help\n> - * gcc with -Wuninitialized analysis. We have to restrict this trick to\n> - * gcc, though, because of the variadic macro and the magic ## comma pasting\n> - * behavior. But since we're only trying to help gcc, anyway, it's OK; other\n> - * compilers will fall back to using the function as usual.\n> + * gcc with -Wuninitialized analysis. We restrict this trick to gcc, though,\n> + * because some compilers may not support variadic macros. Since we're only\n> + * trying to help gcc, anyway, it's OK; other compilers will fall back to\n> + * using the function as usual.\n> */\n> #if defined(__GNUC__) && ! defined(__clang__)\n> -#define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)\n> +#define error(...) (error(__VA_ARGS__), -1)\n\nAcked-by: Jeff King \n\nThanks.\n\n-Peff\n"}]}