{"thread":{"id":"32650","subject":"[PATCH] fix some clang warnings","startedAt":"2013-01-16T14:53:23Z","lastAt":"2013-02-01T05:37:59Z","messageCount":35,"participants":["Max Horn","Jeff King","Junio C Hamano","Antoine Pelisse","John Keeping","Tomas Carnecky","Matthieu Moy","Linus Torvalds","Phil Hord","Miles Bader"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"207054","messageId":"1358348003-11130-1-git-send-email-max@quendi.de","threadId":"32650","inReplyTo":null,"subject":"[PATCH] fix some clang warnings","fromName":"Max Horn","fromEmail":"max@quendi.de","sentAt":"2013-01-16T14:53:23Z","receivedAt":"2013-01-16T14:53:23Z","isPatch":true,"sender":{"key":"max@quendi.de","avatar":"https://avatars.githubusercontent.com/u/241512?v=4"},"body":"\nSigned-off-by: Max Horn \n---\n cache.h | 2 +-\n git-compat-util.h | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex c257953..5c8440b 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1148,7 +1148,7 @@ extern int check_repository_format_version(const char *var, const char *value, v\n extern int git_env_bool(const char *, int);\n extern int git_config_system(void);\n extern int config_error_nonbool(const char *);\n-#ifdef __GNUC__\n+#if defined(__GNUC__) && ! defined(__clang__)\n #define config_error_nonbool(s) (config_error_nonbool(s), -1)\n #endif\n extern const char *get_log_output_encoding(void);\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 4f022a3..cc2abee 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -310,7 +310,7 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)))\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 */\n-#ifdef __GNUC__\n+#if defined(__GNUC__) && ! defined(__clang__)\n #define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)\n #endif\n \n-- \n1.8.1.1.435.g4e2ebdf\n"},{"id":"207064","messageId":"20130116160410.GC22400@sigill.intra.peff.net","threadId":"32650","inReplyTo":"1358348003-11130-1-git-send-email-max@quendi.de","subject":"Re: [PATCH] fix some clang warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-16T16:04:10Z","receivedAt":"2013-01-16T16:04:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 16, 2013 at 03:53:23PM +0100, Max Horn wrote:\n\n> -#ifdef __GNUC__\n> +#if defined(__GNUC__) && ! defined(__clang__)\n> #define config_error_nonbool(s) (config_error_nonbool(s), -1)\n> #endif\n\nYou don't say what the warning is, but I'm guessing it's complaining\nabout throwing away the return value from config_error_nonbool?\n\n-Peff\n"},{"id":"207066","messageId":"7vk3rdxe5y.fsf@alter.siamese.dyndns.org","threadId":"32650","inReplyTo":"20130116160410.GC22400@sigill.intra.peff.net","subject":"Re: [PATCH] fix some clang warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-16T16:53:29Z","receivedAt":"2013-01-16T16:53:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Jan 16, 2013 at 03:53:23PM +0100, Max Horn wrote:\n>\n>> -#ifdef __GNUC__\n>> +#if defined(__GNUC__) && ! defined(__clang__)\n>> #define config_error_nonbool(s) (config_error_nonbool(s), -1)\n>> #endif\n>\n> You don't say what the warning is, but I'm guessing it's complaining\n> about throwing away the return value from config_error_nonbool?\n\nYeah, I was wondering about the same thing. The other one looks\nsimilar, ignoring the return value of error().\n\nAlso, is this \"some versions of clang do not like this\"? Or are all\nversions of clang affected?\n \n"},{"id":"207070","messageId":"CALWbr2z4TiynwOR3Lk4005dbZaLtcHK3J01ZF73wp8Q7Rm6YBA@mail.gmail.com","threadId":"32650","inReplyTo":"7vk3rdxe5y.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] fix some clang warnings","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-01-16T17:12:57Z","receivedAt":"2013-01-16T17:12:57Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"FWIW, I also happen to have the warning:\n\nadvice.c:69:2: warning: expression result unused [-Wunused-value]\n error(\"'%s' is not possible because you have unmerged files.\", me);\n ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n./git-compat-util.h:314:55: note: expanded from:\n#define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)\n ^~\n\nwith clang: Ubuntu clang version 3.0-6ubuntu3 (tags/RELEASE_30/final)\n(based on LLVM 3.0)\n\nI can't say about other versions.\n\nOn Wed, Jan 16, 2013 at 5:53 PM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> On Wed, Jan 16, 2013 at 03:53:23PM +0100, Max Horn wrote:\n>>\n>>> -#ifdef __GNUC__\n>>> +#if defined(__GNUC__) && ! defined(__clang__)\n>>> #define config_error_nonbool(s) (config_error_nonbool(s), -1)\n>>> #endif\n>>\n>> You don't say what the warning is, but I'm guessing it's complaining\n>> about throwing away the return value from config_error_nonbool?\n>\n> Yeah, I was wondering about the same thing. The other one looks\n> similar, ignoring the return value of error().\n>\n> Also, is this \"some versions of clang do not like this\"? Or are all\n> versions of clang affected?\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"207071","messageId":"20130116171809.GA2476@farnsworth.metanate.com","threadId":"32650","inReplyTo":"CALWbr2z4TiynwOR3Lk4005dbZaLtcHK3J01ZF73wp8Q7Rm6YBA@mail.gmail.com","subject":"Re: [PATCH] fix some clang warnings","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-16T17:18:09Z","receivedAt":"2013-01-16T17:18:09Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Wed, Jan 16, 2013 at 06:12:57PM +0100, Antoine Pelisse wrote:\n> FWIW, I also happen to have the warning:\n> \n> advice.c:69:2: warning: expression result unused [-Wunused-value]\n> error(\"'%s' is not possible because you have unmerged files.\", me);\n> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n> ./git-compat-util.h:314:55: note: expanded from:\n> #define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)\n> ^~\n> \n> with clang: Ubuntu clang version 3.0-6ubuntu3 (tags/RELEASE_30/final)\n> (based on LLVM 3.0)\n\nI have the same output with:\n\nclang version 3.2 (tags/RELEASE_32/final)\n"},{"id":"207072","messageId":"7FDA1B56-731E-4BA2-8FE5-196B965FFFDB@quendi.de","threadId":"32650","inReplyTo":"20130116171809.GA2476@farnsworth.metanate.com","subject":"Re: [PATCH] fix some clang warnings","fromName":"Max Horn","fromEmail":"max@quendi.de","sentAt":"2013-01-16T17:26:35Z","receivedAt":"2013-01-16T17:26:35Z","isPatch":true,"sender":{"key":"max@quendi.de","avatar":"https://avatars.githubusercontent.com/u/241512?v=4"},"body":"\nOn 16.01.2013, at 18:18, John Keeping wrote:\n\n> On Wed, Jan 16, 2013 at 06:12:57PM +0100, Antoine Pelisse wrote:\n>> FWIW, I also happen to have the warning:\n>> \n>> advice.c:69:2: warning: expression result unused [-Wunused-value]\n>> error(\"'%s' is not possible because you have unmerged files.\", me);\n>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n>> ./git-compat-util.h:314:55: note: expanded from:\n>> #define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)\n>> ^~\n>> \n>> with clang: Ubuntu clang version 3.0-6ubuntu3 (tags/RELEASE_30/final)\n>> (based on LLVM 3.0)\n> \n> I have the same output with:\n> \n> clang version 3.2 (tags/RELEASE_32/final)\n\nSorry for not being more specific in my message. I have this with \n\nApple clang version 4.1 (tags/Apple/clang-421.11.66) (based on LLVM 3.1svn)\n\n\nMax\n"},{"id":"207078","messageId":"20130116175057.GB27525@sigill.intra.peff.net","threadId":"32650","inReplyTo":"7FDA1B56-731E-4BA2-8FE5-196B965FFFDB@quendi.de","subject":"Re: [PATCH] fix some clang warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-16T17:50:57Z","receivedAt":"2013-01-16T17:50:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 16, 2013 at 06:26:35PM +0100, Max Horn wrote:\n\n> > On Wed, Jan 16, 2013 at 06:12:57PM +0100, Antoine Pelisse wrote:\n> >> FWIW, I also happen to have the warning:\n> >> \n> >> advice.c:69:2: warning: expression result unused [-Wunused-value]\n> >> error(\"'%s' is not possible because you have unmerged files.\", me);\n> >> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n> >> ./git-compat-util.h:314:55: note: expanded from:\n> >> #define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)\n> >> ^~\n> >> \n> >> with clang: Ubuntu clang version 3.0-6ubuntu3 (tags/RELEASE_30/final)\n> >> (based on LLVM 3.0)\n> > \n> > I have the same output with:\n> > \n> > clang version 3.2 (tags/RELEASE_32/final)\n> \n> Sorry for not being more specific in my message. I have this with \n> \n> Apple clang version 4.1 (tags/Apple/clang-421.11.66) (based on LLVM 3.1svn)\n\nSo it seems pretty common, and is just that clang is more concerned\nabout this than gcc. I think your patch is a reasonable workaround. It\nseems a little weird to me that clang defines __GNUC__, but I assume\nthere are good reasons for it. The commit message should probably be\nalong the lines of:\n\n Commit a469a10 wraps some error calls in macros to give the compiler a\n chance to do more static analysis on their constant -1 return value.\n We limit the use of these macros to __GNUC__, since gcc is the primary\n beneficiary of the new information, and because we use GNU features\n for handling variadic macros.\n\n However, clang also defines __GNUC__, but generates warnings (due to\n throwing away the return value from the first half of the macro). We\n can squelch the warning by turning off these macros when clang is in\n use.\n\nI'm confused, though, why your patch does not have a matching update to\nthe opterror macro in parse-options.h. It uses exactly the same\ntechnique. Does it not generate a warning?\n\n-Peff\n"},{"id":"207080","messageId":"20130116180041.GC27525@sigill.intra.peff.net","threadId":"32650","inReplyTo":"20130116175057.GB27525@sigill.intra.peff.net","subject":"Re: [PATCH] fix some clang warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-16T18:00:42Z","receivedAt":"2013-01-16T18:00:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 16, 2013 at 09:50:57AM -0800, Jeff King wrote:\n\n> I'm confused, though, why your patch does not have a matching update to\n> the opterror macro in parse-options.h. It uses exactly the same\n> technique. Does it not generate a warning?\n\nAh, I think I see why not.\n\nIt is not about the macro itself, but rather the callsites that do not\nreturn error, but call it for its printing side effect. It seems that\nclang -Wunused-value is OK with unused values from functions being\ndiscarded, but not with constants. So:\n\n int foo();\n void bar()\n {\n foo(); /* ok */\n 1; /* not ok */\n (foo(), 1); /* not ok */\n }\n\nThe first one is OK (I think it would fall under -Wunused-result under\neither compiler). The middle one is an obvious error, and caught by both\ncompilers. The last one is OK by gcc, but clang complains.\n\nSo opterror does not happen to generate any warnings, because we do not\never use it in a void context. It should probably be marked the same\nway, though, as future-proofing.\n\n> The commit message should probably be along the lines of:\n> [...]\n> However, clang also defines __GNUC__, but generates warnings (due to\n> throwing away the return value from the first half of the macro). We\n> can squelch the warning by turning off these macros when clang is in\n> use.\n\nSo a more accurate description would be:\n\n However, clang also defines __GNUC__, but generates warnings with\n -Wunused-value when these macros are used in a void context, because\n the constant \"-1\" ends up being useless. Gcc does not complain about\n this case (though it is unclear if it is because it is smart enough to\n see what we are doing, or too dumb to realize that the -1 is unused).\n We can squelch the warning by just disabling these macros when clang\n is in use.\n\n-Peff\n"},{"id":"207081","messageId":"1358359395-ner-7610@calvin","threadId":"32650","inReplyTo":"20130116175057.GB27525@sigill.intra.peff.net","subject":"Re: [PATCH] fix some clang warnings","fromName":"Tomas Carnecky","fromEmail":"tomas.carnecky@gmail.com","sentAt":"2013-01-16T18:03:15Z","receivedAt":"2013-01-16T18:03:15Z","isPatch":true,"sender":{"key":"tomas.carnecky@gmail.com","avatar":null},"body":"On Wed, 16 Jan 2013 09:50:57 -0800, Jeff King wrote:\n> However, clang also defines __GNUC__, [...]\n\nhttp://sourceforge.net/p/predef/wiki/Compilers/\n\n Notice that the meaning of the __GNUC__ macro has changed subtly over the\n years, from identifying the GNU C/C++ compiler to identifying any compiler\n that implements the GNU compiler extensions (...). For example, the Intel\n C++ on Linux also defines these macros from version 8.1 (...).\n"},{"id":"207084","messageId":"20130116180929.GD27525@sigill.intra.peff.net","threadId":"32650","inReplyTo":"20130116180041.GC27525@sigill.intra.peff.net","subject":"Re: [PATCH] fix some clang warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-16T18:09:29Z","receivedAt":"2013-01-16T18:09:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 16, 2013 at 10:00:42AM -0800, Jeff King wrote:\n\n> So opterror does not happen to generate any warnings, because we do not\n> ever use it in a void context. It should probably be marked the same\n> way, though, as future-proofing.\n> [...]\n> So a more accurate description would be:\n\nHere it is all together:\n\n-- >8 --\nFrom: Max Horn \nSubject: [PATCH] fix clang -Wunused-value warnings for error functions\n\nCommit a469a10 wraps some error calls in macros to give the\ncompiler a chance to do more static analysis on their\nconstant -1 return value. We limit the use of these macros\nto __GNUC__, since gcc is the primary beneficiary of the new\ninformation, and because we use GNU features for handling\nvariadic macros.\n\nHowever, clang also defines __GNUC__, but generates warnings\nwith -Wunused-value when these macros are used in a void\ncontext, because the constant \"-1\" ends up being useless.\nGcc does not complain about this case (though it is unclear\nif it is because it is smart enough to see what we are\ndoing, or too dumb to realize that the -1 is unused). We\ncan squelch the warning by just disabling these macros when\nclang is in use.\n\nSigned-off-by: Max Horn \nSigned-off-by: Jeff King \n---\n cache.h | 2 +-\n git-compat-util.h | 2 +-\n parse-options.h | 2 +-\n 3 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex c257953..5c8440b 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1148,7 +1148,7 @@ extern int config_error_nonbool(const char *);\n extern int git_env_bool(const char *, int);\n extern int git_config_system(void);\n extern int config_error_nonbool(const char *);\n-#ifdef __GNUC__\n+#if defined(__GNUC__) && ! defined(__clang__)\n #define config_error_nonbool(s) (config_error_nonbool(s), -1)\n #endif\n extern const char *get_log_output_encoding(void);\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 2cecf56..2596280 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -297,7 +297,7 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)))\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 */\n-#ifdef __GNUC__\n+#if defined(__GNUC__) && ! defined(__clang__)\n #define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)\n #endif\n \ndiff --git a/parse-options.h b/parse-options.h\nindex e703853..1c8bd8d 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -177,7 +177,7 @@ extern int opterror(const struct option *opt, const char *reason, int flags);\n \n extern int optbug(const struct option *opt, const char *reason);\n extern int opterror(const struct option *opt, const char *reason, int flags);\n-#ifdef __GNUC__\n+#if defined(__GNUC__) && ! defined(clang)\n #define opterror(o,r,f) (opterror((o),(r),(f)), -1)\n #endif\n \n-- \n1.8.1.rc1.10.g7d71f7b\n"},{"id":"207085","messageId":"20130116181203.GB2476@farnsworth.metanate.com","threadId":"32650","inReplyTo":"20130116180041.GC27525@sigill.intra.peff.net","subject":"Re: [PATCH] fix some clang warnings","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-16T18:12:03Z","receivedAt":"2013-01-16T18:12:03Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Wed, Jan 16, 2013 at 10:00:42AM -0800, Jeff King wrote:\n> It is not about the macro itself, but rather the callsites that do not\n> return error, but call it for its printing side effect. It seems that\n> clang -Wunused-value is OK with unused values from functions being\n> discarded, but not with constants. So:\n> \n> int foo();\n> void bar()\n> {\n> foo(); /* ok */\n> 1; /* not ok */\n> (foo(), 1); /* not ok */\n> }\n> \n> The first one is OK (I think it would fall under -Wunused-result under\n> either compiler). The middle one is an obvious error, and caught by both\n> compilers. The last one is OK by gcc, but clang complains.\n\nI wonder if this would be changed in clang - the change in [1] is\nsuperficially similar.\n\n[1] http://llvm.org/bugs/show_bug.cgi?id=13747\n"},{"id":"207086","messageId":"vpqr4llhua3.fsf@grenoble-inp.fr","threadId":"32650","inReplyTo":"20130116175057.GB27525@sigill.intra.peff.net","subject":"Re: [PATCH] fix some clang warnings","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-01-16T18:12:04Z","receivedAt":"2013-01-16T18:12:04Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Jeff King writes:\n\n> It seems a little weird to me that clang defines __GNUC__, but I\n> assume there are good reasons for it.\n\nThe reason is essentially that clang targets compatibility with GCC\n(implementing the same extensions & cie), in the sense \"drop in\nreplacement that should be able to compile legacy code possibly relying\non __GNUC__ and GCC extensions.\"\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"207087","messageId":"20130116181558.GA4426@sigill.intra.peff.net","threadId":"32650","inReplyTo":"20130116181203.GB2476@farnsworth.metanate.com","subject":"Re: [PATCH] fix some clang warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-16T18:15:58Z","receivedAt":"2013-01-16T18:15:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 16, 2013 at 06:12:03PM +0000, John Keeping wrote:\n\n> On Wed, Jan 16, 2013 at 10:00:42AM -0800, Jeff King wrote:\n> > It is not about the macro itself, but rather the callsites that do not\n> > return error, but call it for its printing side effect. It seems that\n> > clang -Wunused-value is OK with unused values from functions being\n> > discarded, but not with constants. So:\n> > \n> > int foo();\n> > void bar()\n> > {\n> > foo(); /* ok */\n> > 1; /* not ok */\n> > (foo(), 1); /* not ok */\n> > }\n> > \n> > The first one is OK (I think it would fall under -Wunused-result under\n> > either compiler). The middle one is an obvious error, and caught by both\n> > compilers. The last one is OK by gcc, but clang complains.\n> \n> I wonder if this would be changed in clang - the change in [1] is\n> superficially similar.\n> \n> [1] http://llvm.org/bugs/show_bug.cgi?id=13747\n\nYeah, I think it is exactly the same issue, and the fix they mention\nthere would apply to us, too.\n\nIs it worth applying this at all, then? Or should we apply it but limit\nit with a clang version macro (they mention r163034, but I do not know\nif it is in a released version yet, nor what macros are available to\ninspect the version)?\n\n-Peff\n"},{"id":"207088","messageId":"CALWbr2yz1LsQBpHW52x-QyQA-5d+R-sMibF+ZT-H=W+0gQNcuA@mail.gmail.com","threadId":"32650","inReplyTo":"20130116181558.GA4426@sigill.intra.peff.net","subject":"Re: [PATCH] fix some clang warnings","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-01-16T18:21:38Z","receivedAt":"2013-01-16T18:21:38Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"> Is it worth applying this at all, then? Or should we apply it but limit\n> it with a clang version macro (they mention r163034, but I do not know\n> if it is in a released version yet, nor what macros are available to\n> inspect the version)?\n\nPlease also note that building with clang is not warning-free (though\nI think it would be nice)\n"},{"id":"207090","messageId":"20130116182240.GC2476@farnsworth.metanate.com","threadId":"32650","inReplyTo":"20130116181558.GA4426@sigill.intra.peff.net","subject":"Re: [PATCH] fix some clang warnings","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-16T18:22:40Z","receivedAt":"2013-01-16T18:22:40Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Wed, Jan 16, 2013 at 10:15:58AM -0800, Jeff King wrote:\n> On Wed, Jan 16, 2013 at 06:12:03PM +0000, John Keeping wrote:\n> \n> > On Wed, Jan 16, 2013 at 10:00:42AM -0800, Jeff King wrote:\n> > > It is not about the macro itself, but rather the callsites that do not\n> > > return error, but call it for its printing side effect. It seems that\n> > > clang -Wunused-value is OK with unused values from functions being\n> > > discarded, but not with constants. So:\n> > > \n> > > int foo();\n> > > void bar()\n> > > {\n> > > foo(); /* ok */\n> > > 1; /* not ok */\n> > > (foo(), 1); /* not ok */\n> > > }\n> > > \n> > > The first one is OK (I think it would fall under -Wunused-result under\n> > > either compiler). The middle one is an obvious error, and caught by both\n> > > compilers. The last one is OK by gcc, but clang complains.\n> > \n> > I wonder if this would be changed in clang - the change in [1] is\n> > superficially similar.\n> > \n> > [1] http://llvm.org/bugs/show_bug.cgi?id=13747\n> \n> Yeah, I think it is exactly the same issue, and the fix they mention\n> there would apply to us, too.\n> \n> Is it worth applying this at all, then? Or should we apply it but limit\n> it with a clang version macro (they mention r163034, but I do not know\n> if it is in a released version yet, nor what macros are available to\n> inspect the version)?\n\nThat maps to revision 06b3a06007 in their git repository [1], which is\ncontained in remotes/origin/release_32 so I think that change should be\nin release 3.2, where I still see the warning (although that's not using\na clang built from that source), so I don't think that the fix for that\nbug removes the warning in this case.\n\n[1] http://llvm.org/git/clang.git\n"},{"id":"207092","messageId":"20130116182449.GA4881@sigill.intra.peff.net","threadId":"32650","inReplyTo":"20130116182240.GC2476@farnsworth.metanate.com","subject":"Re: [PATCH] fix some clang warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-16T18:24:49Z","receivedAt":"2013-01-16T18:24:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 16, 2013 at 06:22:40PM +0000, John Keeping wrote:\n\n> > > [1] http://llvm.org/bugs/show_bug.cgi?id=13747\n> > \n> > Yeah, I think it is exactly the same issue, and the fix they mention\n> > there would apply to us, too.\n> > \n> > Is it worth applying this at all, then? Or should we apply it but limit\n> > it with a clang version macro (they mention r163034, but I do not know\n> > if it is in a released version yet, nor what macros are available to\n> > inspect the version)?\n> \n> That maps to revision 06b3a06007 in their git repository [1], which is\n> contained in remotes/origin/release_32 so I think that change should be\n> in release 3.2, where I still see the warning (although that's not using\n> a clang built from that source), so I don't think that the fix for that\n> bug removes the warning in this case.\n> \n> [1] http://llvm.org/git/clang.git\n\nThanks for checking. I'd rather squelch the warning completely (as in my\nre-post of Max's patch from a few minutes ago), and we can loosen it\n(possibly with a version check) later when a fix is widely disseminated.\n\nI know that compiling git with clang is not warning-free yet, but it is\nclose, and I do not mind if somebody puts some effort into making it so.\nThis gets us one step closer.\n\n-Peff\n"},{"id":"207094","messageId":"20130116190137.GD2476@farnsworth.metanate.com","threadId":"32650","inReplyTo":"20130116182449.GA4881@sigill.intra.peff.net","subject":"Re: [PATCH] fix some clang warnings","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-16T19:01:37Z","receivedAt":"2013-01-16T19:01:37Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Wed, Jan 16, 2013 at 10:24:49AM -0800, Jeff King wrote:\n> On Wed, Jan 16, 2013 at 06:22:40PM +0000, John Keeping wrote:\n> \n> > > > [1] http://llvm.org/bugs/show_bug.cgi?id=13747\n> > > \n> > > Yeah, I think it is exactly the same issue, and the fix they mention\n> > > there would apply to us, too.\n> > > \n> > > Is it worth applying this at all, then? Or should we apply it but limit\n> > > it with a clang version macro (they mention r163034, but I do not know\n> > > if it is in a released version yet, nor what macros are available to\n> > > inspect the version)?\n> > \n> > That maps to revision 06b3a06007 in their git repository [1], which is\n> > contained in remotes/origin/release_32 so I think that change should be\n> > in release 3.2, where I still see the warning (although that's not using\n> > a clang built from that source), so I don't think that the fix for that\n> > bug removes the warning in this case.\n> > \n> > [1] http://llvm.org/git/clang.git\n> \n> Thanks for checking. I'd rather squelch the warning completely (as in my\n> re-post of Max's patch from a few minutes ago), and we can loosen it\n> (possibly with a version check) later when a fix is widely disseminated.\n\nI checked again with a trunk build of clang and the warning's still\nthere, so I've created a clang bug [1] to see if they will change the\nbehaviour.\n\nI agree that we should squelch the warning for now, it can be changed\ninto a version check if it's accepted as a bug and once we know what\nversion it's fixed in.\n\n[1] http://llvm.org/bugs/show_bug.cgi?id=14968\n"},{"id":"207112","messageId":"1358376443-7404-1-git-send-email-apelisse@gmail.com","threadId":"32650","inReplyTo":"20130116182449.GA4881@sigill.intra.peff.net","subject":"[PATCH 1/2] fix clang -Wconstant-conversion with bit fields","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-01-16T22:47:22Z","receivedAt":"2013-01-16T22:47:22Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"clang incorrectly reports a constant conversion warning (implicit\ntruncation to bit field) when using the \"flag &= ~FLAG\" form, because\n~FLAG needs to be truncated.\n\nConvert this form to \"flag = flag & ~FLAG\" fixes the issue as\nthe right operand now fits into the bit field.\n\nSigned-off-by: Antoine Pelisse \n---\nI'm sorry about this fix, it really seems bad, yet it's one step closer\nto warning-free clang compilation.\n\nIt seems quite clear to me that it's a bug in clang.\n\n bisect.c | 2 +-\n builtin/checkout.c | 2 +-\n builtin/reflog.c | 4 ++--\n commit.c | 4 ++--\n revision.c | 8 ++++----\n upload-pack.c | 4 ++--\n 6 files changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex bd1b7b5..34ac01d 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -63,7 +63,7 @@ static void clear_distance(struct commit_list *list)\n {\n \twhile (list) {\n \t\tstruct commit *commit = list->item;\n-\t\tcommit->object.flags &= ~COUNTED;\n+\t\tcommit->object.flags = commit->object.flags & ~COUNTED;\n \t\tlist = list->next;\n \t}\n }\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex a9c1b5a..2c83234 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -717,7 +717,7 @@ static void orphaned_commit_warning(struct commit *old, struct commit *new)\n \tinit_revisions(&revs, NULL);\n \tsetup_revisions(0, NULL, &revs, NULL);\n\n-\tobject->flags &= ~UNINTERESTING;\n+\tobject->flags = object->flags & ~UNINTERESTING;\n \tadd_pending_object(&revs, object, sha1_to_hex(object->sha1));\n\n \tfor_each_ref(add_pending_uninteresting_ref, &revs);\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex b3c9e27..3079c81 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -170,7 +170,7 @@ static int commit_is_complete(struct commit *commit)\n \t}\n \t/* clear flags from the objects we traversed */\n \tfor (i = 0; i < found.nr; i++)\n-\t\tfound.objects[i].item->flags &= ~STUDYING;\n+\t\tfound.objects[i].item->flags = found.objects[i].item->flags& ~STUDYING;\n \tif (is_incomplete)\n \t\tcommit->object.flags |= INCOMPLETE;\n \telse {\n@@ -229,7 +229,7 @@ static void mark_reachable(struct expire_reflog_cb *cb)\n \tstruct commit_list *leftover = NULL;\n\n \tfor (pending = cb->mark_list; pending; pending = pending->next)\n-\t\tpending->item->object.flags &= ~REACHABLE;\n+\t\tpending->item->object.flags = pending->item->object.flags & ~REACHABLE;\n\n \tpending = cb->mark_list;\n \twhile (pending) {\ndiff --git a/commit.c b/commit.c\nindex e8eb0ae..800779d 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -883,7 +883,7 @@ struct commit_list *reduce_heads(struct commit_list *heads)\n\n \t/* Uniquify */\n \tfor (p = heads; p; p = p->next)\n-\t\tp->item->object.flags &= ~STALE;\n+\t\tp->item->object.flags = p->item->object.flags & ~STALE;\n \tfor (p = heads, num_head = 0; p; p = p->next) {\n \t\tif (p->item->object.flags & STALE)\n \t\t\tcontinue;\n@@ -894,7 +894,7 @@ struct commit_list *reduce_heads(struct commit_list *heads)\n \tfor (p = heads, i = 0; p; p = p->next) {\n \t\tif (p->item->object.flags & STALE) {\n \t\t\tarray[i++] = p->item;\n-\t\t\tp->item->object.flags &= ~STALE;\n+\t\t\tp->item->object.flags = p->item->object.flags & ~STALE;\n \t\t}\n \t}\n \tnum_head = remove_redundant(array, num_head);\ndiff --git a/revision.c b/revision.c\nindex d7562ee..ed1c16d 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -787,9 +787,9 @@ static void limit_to_ancestry(struct commit_list *bottom, struct commit_list *li\n\n \t/* We are done with the TMP_MARK */\n \tfor (p = list; p; p = p->next)\n-\t\tp->item->object.flags &= ~TMP_MARK;\n+\t\tp->item->object.flags = p->item->object.flags & ~TMP_MARK;\n \tfor (p = bottom; p; p = p->next)\n-\t\tp->item->object.flags &= ~TMP_MARK;\n+\t\tp->item->object.flags = p->item->object.flags & ~TMP_MARK;\n \tfree_commit_list(rlist);\n }\n\n@@ -1948,7 +1948,7 @@ static int remove_duplicate_parents(struct commit *commit)\n \t/* count them while clearing the temporary mark */\n \tsurviving_parents = 0;\n \tfor (p = commit->parents; p; p = p->next) {\n-\t\tp->item->object.flags &= ~TMP_MARK;\n+\t\tp->item->object.flags = p->item->object.flags & ~TMP_MARK;\n \t\tsurviving_parents++;\n \t}\n \treturn surviving_parents;\n@@ -2378,7 +2378,7 @@ static struct commit *get_revision_1(struct rev_info *revs)\n\n \t\tif (revs->reflog_info) {\n \t\t\tfake_reflog_parent(revs->reflog_info, commit);\n-\t\t\tcommit->object.flags &= ~(ADDED | SEEN | SHOWN);\n+\t\t\tcommit->object.flags = commit->object.flags & ~(ADDED | SEEN | SHOWN);\n \t\t}\n\n \t\t/*\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 7c05b15..74d8f0e 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -113,7 +113,7 @@ static int do_rev_list(int in, int out, void *user_data)\n \tfor (i = 0; i < want_obj.nr; i++) {\n \t\tstruct object *o = want_obj.objects[i].item;\n \t\t/* why??? */\n-\t\to->flags &= ~UNINTERESTING;\n+\t\to->flags = o->flags & ~UNINTERESTING;\n \t\tadd_pending_object(&revs, o, NULL);\n \t}\n \tfor (i = 0; i < have_obj.nr; i++) {\n@@ -700,7 +700,7 @@ static void receive_needs(void)\n \t\t\t\tstruct commit_list *parents;\n \t\t\t\tpacket_write(1, \"unshallow %s\",\n \t\t\t\t\tsha1_to_hex(object->sha1));\n-\t\t\t\tobject->flags &= ~CLIENT_SHALLOW;\n+\t\t\t\tobject->flags = object->flags & ~CLIENT_SHALLOW;\n \t\t\t\t/* make sure the real parents are parsed */\n \t\t\t\tunregister_shallow(object->sha1);\n \t\t\t\tobject->parsed = 0;\n--\n1.8.1.1.435.g20d29be.dirty\n"},{"id":"207111","messageId":"1358376443-7404-2-git-send-email-apelisse@gmail.com","threadId":"32650","inReplyTo":"1358376443-7404-1-git-send-email-apelisse@gmail.com","subject":"[PATCH 2/2] fix clang -Wtautological-compare with unsigned enum","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-01-16T22:47:23Z","receivedAt":"2013-01-16T22:47:23Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"Create a GREP_HEADER_FIELD_MIN so we can check that the field value is\nsane and silent the clang warning.\n\nClang warning happens because the enum is unsigned (this is\nimplementation-defined, and there is no negative fields) and the check\nis then tautological.\n\nSigned-off-by: Antoine Pelisse \n---\nI tried to consider discussion [1] and this [2] discussion on clang's list\n\nWith these two patches and the patch from Max Horne, I'm finally able to\ncompile with CC=clang CFLAGS=-Werror.\n\n [1]: http://thread.gmane.org/gmane.comp.version-control.git/184908\n [2]: http://clang-developers.42468.n3.nabble.com/Possibly-invalid-enum-tautology-warning-td3233140.html\n\n grep.c | 3 ++-\n grep.h | 3 ++-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 4bd1b8b..bb548ca 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -625,7 +625,8 @@ static struct grep_expr *prep_header_patterns(struct grep_opt *opt)\n \tfor (p = opt->header_list; p; p = p->next) {\n \t\tif (p->token != GREP_PATTERN_HEAD)\n \t\t\tdie(\"bug: a non-header pattern in grep header list.\");\n-\t\tif (p->field < 0 || GREP_HEADER_FIELD_MAX <= p->field)\n+\t\tif (p->field < GREP_HEADER_FIELD_MIN ||\n+\t\t GREP_HEADER_FIELD_MAX <= p->field)\n \t\t\tdie(\"bug: unknown header field %d\", p->field);\n \t\tcompile_regexp(p, opt);\n \t}\ndiff --git a/grep.h b/grep.h\nindex 8fc854f..e4a1df5 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -28,7 +28,8 @@ enum grep_context {\n };\n\n enum grep_header_field {\n-\tGREP_HEADER_AUTHOR = 0,\n+\tGREP_HEADER_FIELD_MIN = 0,\n+\tGREP_HEADER_AUTHOR = GREP_HEADER_FIELD_MIN,\n \tGREP_HEADER_COMMITTER,\n \tGREP_HEADER_REFLOG,\n\n--\n1.8.1.1.435.g20d29be.dirty\n"},{"id":"207115","messageId":"20130116230800.GB4574@serenity.lan","threadId":"32650","inReplyTo":"1358376443-7404-1-git-send-email-apelisse@gmail.com","subject":"Re: [PATCH 1/2] fix clang -Wconstant-conversion with bit fields","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-16T23:08:00Z","receivedAt":"2013-01-16T23:08:00Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Wed, Jan 16, 2013 at 11:47:22PM +0100, Antoine Pelisse wrote:\n> clang incorrectly reports a constant conversion warning (implicit\n> truncation to bit field) when using the \"flag &= ~FLAG\" form, because\n> ~FLAG needs to be truncated.\n> \n> Convert this form to \"flag = flag & ~FLAG\" fixes the issue as\n> the right operand now fits into the bit field.\n> \n> Signed-off-by: Antoine Pelisse \n> ---\n> I'm sorry about this fix, it really seems bad, yet it's one step closer\n> to warning-free clang compilation.\n> \n> It seems quite clear to me that it's a bug in clang.\n\nWhich version of clang did you see this with? I don't get these\nwarnings with clang 3.2.\n\n\nJohn\n"},{"id":"207116","messageId":"CALWbr2ypYUvuE4pWfcVvVcnJkRvCNrM1gVHp_UXeke9gbgoE3A@mail.gmail.com","threadId":"32650","inReplyTo":"20130116230800.GB4574@serenity.lan","subject":"Re: [PATCH 1/2] fix clang -Wconstant-conversion with bit fields","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-01-16T23:09:47Z","receivedAt":"2013-01-16T23:09:47Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"On Thu, Jan 17, 2013 at 12:08 AM, John Keeping wrote:\n> On Wed, Jan 16, 2013 at 11:47:22PM +0100, Antoine Pelisse wrote:\n>> clang incorrectly reports a constant conversion warning (implicit\n>> truncation to bit field) when using the \"flag &= ~FLAG\" form, because\n>> ~FLAG needs to be truncated.\n> Which version of clang did you see this with? I don't get these\n> warnings with clang 3.2.\n\nUbuntu clang version 3.0-6ubuntu3 (tags/RELEASE_30/final) (based on LLVM 3.0)\n\nIt's good to know it's been fixed !\n"},{"id":"207117","messageId":"CALWbr2ztJi209w82Kmu876QB9tSdheO8zgNXTv_8USR4eRX8Sw@mail.gmail.com","threadId":"32650","inReplyTo":"1358376443-7404-2-git-send-email-apelisse@gmail.com","subject":"Re: [PATCH 2/2] fix clang -Wtautological-compare with unsigned enum","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-01-16T23:10:11Z","receivedAt":"2013-01-16T23:10:11Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"> With these two patches and the patch from Max Horne,\n\nI'm deeply sorry for this typo Max\n"},{"id":"207118","messageId":"CALWbr2zLZpCtcszihHcbiu5vwP6ezUd3ZLLGw2ck2aykPQpg8w@mail.gmail.com","threadId":"32650","inReplyTo":"CALWbr2ypYUvuE4pWfcVvVcnJkRvCNrM1gVHp_UXeke9gbgoE3A@mail.gmail.com","subject":"Re: [PATCH 1/2] fix clang -Wconstant-conversion with bit fields","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-01-16T23:15:43Z","receivedAt":"2013-01-16T23:15:43Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"So I guess we should drop this patch, it's probably not worth it,\nespecially if it's been fixed already by clang.\n\nOn Thu, Jan 17, 2013 at 12:09 AM, Antoine Pelisse wrote:\n> On Thu, Jan 17, 2013 at 12:08 AM, John Keeping wrote:\n>> On Wed, Jan 16, 2013 at 11:47:22PM +0100, Antoine Pelisse wrote:\n>>> clang incorrectly reports a constant conversion warning (implicit\n>>> truncation to bit field) when using the \"flag &= ~FLAG\" form, because\n>>> ~FLAG needs to be truncated.\n>> Which version of clang did you see this with? I don't get these\n>> warnings with clang 3.2.\n>\n> Ubuntu clang version 3.0-6ubuntu3 (tags/RELEASE_30/final) (based on LLVM 3.0)\n>\n> It's good to know it's been fixed !\n"},{"id":"207120","messageId":"7vpq14vglu.fsf@alter.siamese.dyndns.org","threadId":"32650","inReplyTo":"1358376443-7404-1-git-send-email-apelisse@gmail.com","subject":"Re: [PATCH 1/2] fix clang -Wconstant-conversion with bit fields","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-16T23:43:41Z","receivedAt":"2013-01-16T23:43:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antoine Pelisse writes:\n\n> clang incorrectly reports a constant conversion warning (implicit\n> truncation to bit field) when using the \"flag &= ~FLAG\" form, because\n> ~FLAG needs to be truncated.\n>\n> Convert this form to \"flag = flag & ~FLAG\" fixes the issue as\n> the right operand now fits into the bit field.\n\nIf the \"clang incorrectly reports\" is already recognised by clang\nfolks as a bug to be fixed in clang, I'd rather not to take this\npatch.\n\nI do not think it is reasonable to expect people to remember that\nthey have to write \"flags &= ~TO_DROP\" in a longhand whenever they\nare adding new code that needs to do bit-fields, so even if this\npatch makes clang silent for the _current_ code, it will not stay\nthat way. Something like\n\n#define FLIP_BIT_CLR(fld,bit) do { \\\n\ttypeof(fld) *x = &(fld); \\\n *x = *x & (~(bit)); \\\n} while (0)\n\nmay be more palapable but not by a large margin.\n\nYuck.\n"},{"id":"207121","messageId":"7vlibsvghh.fsf@alter.siamese.dyndns.org","threadId":"32650","inReplyTo":"7vpq14vglu.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] fix clang -Wconstant-conversion with bit fields","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-16T23:46:18Z","receivedAt":"2013-01-16T23:46:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Antoine Pelisse writes:\n>\n>> clang incorrectly reports a constant conversion warning (implicit\n>> truncation to bit field) when using the \"flag &= ~FLAG\" form, because\n>> ~FLAG needs to be truncated.\n>>\n>> Convert this form to \"flag = flag & ~FLAG\" fixes the issue as\n>> the right operand now fits into the bit field.\n>\n> If the \"clang incorrectly reports\" is already recognised by clang\n> folks as a bug to be fixed in clang, I'd rather not to take this\n> patch.\n>\n> I do not think it is reasonable to expect people to remember that\n> they have to write \"flags &= ~TO_DROP\" in a longhand whenever they\n> are adding new code that needs to do bit-fields, so even if this\n> patch makes clang silent for the _current_ code, it will not stay\n> that way. Something like\n>\n> #define FLIP_BIT_CLR(fld,bit) do { \\\n> \ttypeof(fld) *x = &(fld); \\\n> *x = *x & (~(bit)); \\\n> } while (0)\n>\n> may be more palapable but not by a large margin.\n>\n> Yuck.\n\nDouble yuck. I meant palatable.\n\nIn any case, I see somebody reports that more recent clang does not\nhave this bug in the near-by message, so let's forget about this\nissue.\n\nThanks.\n"},{"id":"207139","messageId":"20130117102427.GC4574@serenity.lan","threadId":"32650","inReplyTo":"20130116190137.GD2476@farnsworth.metanate.com","subject":"Re: [PATCH] fix some clang warnings","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-17T10:24:27Z","receivedAt":"2013-01-17T10:24:27Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Wed, Jan 16, 2013 at 07:01:37PM +0000, John Keeping wrote:\n> On Wed, Jan 16, 2013 at 10:24:49AM -0800, Jeff King wrote:\n> > On Wed, Jan 16, 2013 at 06:22:40PM +0000, John Keeping wrote:\n> > \n> > Thanks for checking. I'd rather squelch the warning completely (as in my\n> > re-post of Max's patch from a few minutes ago), and we can loosen it\n> > (possibly with a version check) later when a fix is widely disseminated.\n> \n> I checked again with a trunk build of clang and the warning's still\n> there, so I've created a clang bug [1] to see if they will change the\n> behaviour.\n> \n> [1] http://llvm.org/bugs/show_bug.cgi?id=14968\n\nWell, that was quick! This warning is now gone when using a fresh trunk\nbuild of clang.\n\n>From [2], it looks like this will become version 3.3 (in about 5\nmonths). So should we change the condition to:\n\n#if defined(__GNUC__) && (!defined(__clang__) ||\n\t__clang_major__ > 3 || \\\n (__clang__major == 3 && __clang_minor__ >= 3)\n\n\n[2] http://llvm.org/docs/HowToReleaseLLVM.html\n\n\nJohn\n"},{"id":"207140","messageId":"CALWbr2wk+78zxGKCo-hCOwMuMOzdGspYvMu7PA6o0OYM3Y3m4A@mail.gmail.com","threadId":"32650","inReplyTo":"1358376443-7404-2-git-send-email-apelisse@gmail.com","subject":"Re: [PATCH 2/2] fix clang -Wtautological-compare with unsigned enum","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-01-17T10:32:39Z","receivedAt":"2013-01-17T10:32:39Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"John, could you confirm that you trigger the -Wtautological-compare\nwarning with your version of clang ?\nAnd that this patch makes clang compilation warning-free (with the\nvery latest clang) ?\n\nCheers,\n\nOn Wed, Jan 16, 2013 at 11:47 PM, Antoine Pelisse wrote:\n> Create a GREP_HEADER_FIELD_MIN so we can check that the field value is\n> sane and silent the clang warning.\n>\n> Clang warning happens because the enum is unsigned (this is\n> implementation-defined, and there is no negative fields) and the check\n> is then tautological.\n>\n> Signed-off-by: Antoine Pelisse \n> ---\n> I tried to consider discussion [1] and this [2] discussion on clang's list\n>\n> With these two patches and the patch from Max Horne, I'm finally able to\n> compile with CC=clang CFLAGS=-Werror.\n>\n> [1]: http://thread.gmane.org/gmane.comp.version-control.git/184908\n> [2]: http://clang-developers.42468.n3.nabble.com/Possibly-invalid-enum-tautology-warning-td3233140.html\n>\n> grep.c | 3 ++-\n> grep.h | 3 ++-\n> 2 files changed, 4 insertions(+), 2 deletions(-)\n>\n> diff --git a/grep.c b/grep.c\n> index 4bd1b8b..bb548ca 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -625,7 +625,8 @@ static struct grep_expr *prep_header_patterns(struct grep_opt *opt)\n> for (p = opt->header_list; p; p = p->next) {\n> if (p->token != GREP_PATTERN_HEAD)\n> die(\"bug: a non-header pattern in grep header list.\");\n> - if (p->field < 0 || GREP_HEADER_FIELD_MAX <= p->field)\n> + if (p->field < GREP_HEADER_FIELD_MIN ||\n> + GREP_HEADER_FIELD_MAX <= p->field)\n> die(\"bug: unknown header field %d\", p->field);\n> compile_regexp(p, opt);\n> }\n> diff --git a/grep.h b/grep.h\n> index 8fc854f..e4a1df5 100644\n> --- a/grep.h\n> +++ b/grep.h\n> @@ -28,7 +28,8 @@ enum grep_context {\n> };\n>\n> enum grep_header_field {\n> - GREP_HEADER_AUTHOR = 0,\n> + GREP_HEADER_FIELD_MIN = 0,\n> + GREP_HEADER_AUTHOR = GREP_HEADER_FIELD_MIN,\n> GREP_HEADER_COMMITTER,\n> GREP_HEADER_REFLOG,\n>\n> --\n> 1.8.1.1.435.g20d29be.dirty\n>\n"},{"id":"207141","messageId":"20130117110008.GD4574@serenity.lan","threadId":"32650","inReplyTo":"CALWbr2wk+78zxGKCo-hCOwMuMOzdGspYvMu7PA6o0OYM3Y3m4A@mail.gmail.com","subject":"Re: [PATCH 2/2] fix clang -Wtautological-compare with unsigned enum","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-17T11:00:08Z","receivedAt":"2013-01-17T11:00:08Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Thu, Jan 17, 2013 at 11:32:39AM +0100, Antoine Pelisse wrote:\n> John, could you confirm that you trigger the -Wtautological-compare\n> warning with your version of clang ?\n\nYes, the warning is still there with both 3.2 and a recent trunk build\nbut this patch squelches it.\n\n> And that this patch makes clang compilation warning-free (with the\n> very latest clang) ?\n\nThere is one remaining warning on pu which hasn't been discussed in this\nthread as far as I can see. I'll send a patch shortly.\n\nThere's also a warning that triggers with clang 3.2 but not clang trunk, which\nI think is a legitimate warning - perhaps someone who understands integer type\npromotion better than me can explain why the code is OK (patch->score is\ndeclared as 'int'):\n\nbuiltin/apply.c:1044:47: warning: comparison of constant 18446744073709551615\n with expression of type 'int' is always false\n [-Wtautological-constant-out-of-range-compare]\n if ((patch->score = strtoul(line, NULL, 10)) == ULONG_MAX)\n ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~\n\n\n> On Wed, Jan 16, 2013 at 11:47 PM, Antoine Pelisse wrote:\n> > Create a GREP_HEADER_FIELD_MIN so we can check that the field value is\n> > sane and silent the clang warning.\n> >\n> > Clang warning happens because the enum is unsigned (this is\n> > implementation-defined, and there is no negative fields) and the check\n> > is then tautological.\n> >\n> > Signed-off-by: Antoine Pelisse \n> > ---\n> > I tried to consider discussion [1] and this [2] discussion on clang's list\n> >\n> > With these two patches and the patch from Max Horne, I'm finally able to\n> > compile with CC=clang CFLAGS=-Werror.\n> >\n> > [1]: http://thread.gmane.org/gmane.comp.version-control.git/184908\n> > [2]: http://clang-developers.42468.n3.nabble.com/Possibly-invalid-enum-tautology-warning-td3233140.html\n> >\n> > grep.c | 3 ++-\n> > grep.h | 3 ++-\n> > 2 files changed, 4 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/grep.c b/grep.c\n> > index 4bd1b8b..bb548ca 100644\n> > --- a/grep.c\n> > +++ b/grep.c\n> > @@ -625,7 +625,8 @@ static struct grep_expr *prep_header_patterns(struct grep_opt *opt)\n> > for (p = opt->header_list; p; p = p->next) {\n> > if (p->token != GREP_PATTERN_HEAD)\n> > die(\"bug: a non-header pattern in grep header list.\");\n> > - if (p->field < 0 || GREP_HEADER_FIELD_MAX <= p->field)\n> > + if (p->field < GREP_HEADER_FIELD_MIN ||\n> > + GREP_HEADER_FIELD_MAX <= p->field)\n> > die(\"bug: unknown header field %d\", p->field);\n> > compile_regexp(p, opt);\n> > }\n> > diff --git a/grep.h b/grep.h\n> > index 8fc854f..e4a1df5 100644\n> > --- a/grep.h\n> > +++ b/grep.h\n> > @@ -28,7 +28,8 @@ enum grep_context {\n> > };\n> >\n> > enum grep_header_field {\n> > - GREP_HEADER_AUTHOR = 0,\n> > + GREP_HEADER_FIELD_MIN = 0,\n> > + GREP_HEADER_AUTHOR = GREP_HEADER_FIELD_MIN,\n> > GREP_HEADER_COMMITTER,\n> > GREP_HEADER_REFLOG,\n> >\n> > --\n> > 1.8.1.1.435.g20d29be.dirty\n> >\n"},{"id":"207142","messageId":"20130117112330.GE4574@serenity.lan","threadId":"32650","inReplyTo":"20130117110008.GD4574@serenity.lan","subject":"[PATCH] combine-diff: suppress a clang warning","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-17T11:23:30Z","receivedAt":"2013-01-17T11:23:30Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"When compiling combine-diff.c, clang 3.2 says:\n\n combine-diff.c:1006:19: warning: adding 'int' to a string does not\n\t append to the string [-Wstring-plus-int]\n\t\tprefix = COLONS + offset;\n\t\t\t ~~~~~~~^~~~~~~~\n combine-diff.c:1006:19: note: use array indexing to silence this warning\n\t\tprefix = COLONS + offset;\n\t\t\t\t^\n\t\t\t & [ ]\n\nSuppress this by making the suggested change.\n\nSigned-off-by: John Keeping \n---\nOn Thu, Jan 17, 2013 at 11:00:08AM +0000, John Keeping wrote:\n> On Thu, Jan 17, 2013 at 11:32:39AM +0100, Antoine Pelisse wrote:\n> There is one remaining warning on pu which hasn't been discussed in this\n> thread as far as I can see. I'll send a patch shortly.\n\n... and here it is.\n\n combine-diff.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex bb1cc96..dba4748 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -1003,7 +1003,7 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re\n \t\toffset = strlen(COLONS) - num_parent;\n \t\tif (offset < 0)\n \t\t\toffset = 0;\n-\t\tprefix = COLONS + offset;\n+\t\tprefix = &COLONS[offset];\n \n \t\t/* Show the modes */\n \t\tfor (i = 0; i < num_parent; i++) {\n-- \n1.8.1\n"},{"id":"207150","messageId":"CA+55aFxYSX2iYPSafKdCDSfWSMfQxP3R3Hqh8GuiiR6EbWfk3w@mail.gmail.com","threadId":"32650","inReplyTo":"20130117110008.GD4574@serenity.lan","subject":"Re: [PATCH 2/2] fix clang -Wtautological-compare with unsigned enum","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2013-01-17T16:44:20Z","receivedAt":"2013-01-17T16:44:20Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Thu, Jan 17, 2013 at 3:00 AM, John Keeping wrote:\n>\n> There's also a warning that triggers with clang 3.2 but not clang trunk, which\n> I think is a legitimate warning - perhaps someone who understands integer type\n> promotion better than me can explain why the code is OK (patch->score is\n> declared as 'int'):\n>\n> builtin/apply.c:1044:47: warning: comparison of constant 18446744073709551615\n> with expression of type 'int' is always false\n> [-Wtautological-constant-out-of-range-compare]\n> if ((patch->score = strtoul(line, NULL, 10)) == ULONG_MAX)\n> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~\n\nThe warning seems to be very very wrong, and implies that clang has\nsome nasty bug in it.\n\nSince patch->score is 'int', and UNLONG_MAX is 'unsigned long', the\nconversion rules for the comparison is that the int result from the\nassignment is cast to unsigned long. And if you cast (int)-1 to\nunsigned long, you *do* get ULONG_MAX. That's true regardless of\nwhether \"long\" has the same number of bits as \"int\" or is bigger. The\nimplicit cast will be done as a sign-extension (unsigned long is not\nsigned, but the source type of 'int' *is* signed, and that is what\ndetermines the sign extension on casting).\n\nSo the \"is always false\" is pure and utter crap. clang is wrong, and\nit is wrong in a way that implies that it actually generates incorrect\ncode. It may well be worth making a clang bug report about this.\n\nThat said, clang is certainly understandably confused. The code\ndepends on subtle conversion rules and bit patterns, and is clearly\nvery confusingly written.\n\nSo it would probably be good to rewrite it as\n\n unsigned long val = strtoul(line, NULL, 10);\n if (val == ULONG_MAX) ..\n patch->score = val;\n\ninstead. At which point you might as well make the comparison be \">=\nINT_MAX\" instead, since anything bigger than that is going to be\nbogus.\n\nSo the git code is probably worth cleaning up, but for git it would be\na cleanup. For clang, this implies a major bug and bad code\ngeneration.\n\n Linus\n Linus\n"},{"id":"207151","messageId":"CALWbr2wPQkOyu5cUYq6tDAEA6S9jeykej=F5VomihweckTd3Rw@mail.gmail.com","threadId":"32650","inReplyTo":"CA+55aFxYSX2iYPSafKdCDSfWSMfQxP3R3Hqh8GuiiR6EbWfk3w@mail.gmail.com","subject":"Re: [PATCH 2/2] fix clang -Wtautological-compare with unsigned enum","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-01-17T16:56:38Z","receivedAt":"2013-01-17T16:56:38Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"BTW, I think it has been addressed [1] by clang already and that would\nexplain why you don't have the warning when using clang trunk version.\n\n[1]: http://llvm-reviews.chandlerc.com/D113\n\nAntoine,\n\nOn Thu, Jan 17, 2013 at 5:44 PM, Linus Torvalds\n wrote:\n> On Thu, Jan 17, 2013 at 3:00 AM, John Keeping wrote:\n>>\n>> There's also a warning that triggers with clang 3.2 but not clang trunk, which\n>> I think is a legitimate warning - perhaps someone who understands integer type\n>> promotion better than me can explain why the code is OK (patch->score is\n>> declared as 'int'):\n>>\n>> builtin/apply.c:1044:47: warning: comparison of constant 18446744073709551615\n>> with expression of type 'int' is always false\n>> [-Wtautological-constant-out-of-range-compare]\n>> if ((patch->score = strtoul(line, NULL, 10)) == ULONG_MAX)\n>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~\n>\n> The warning seems to be very very wrong, and implies that clang has\n> some nasty bug in it.\n>\n> Since patch->score is 'int', and UNLONG_MAX is 'unsigned long', the\n> conversion rules for the comparison is that the int result from the\n> assignment is cast to unsigned long. And if you cast (int)-1 to\n> unsigned long, you *do* get ULONG_MAX. That's true regardless of\n> whether \"long\" has the same number of bits as \"int\" or is bigger. The\n> implicit cast will be done as a sign-extension (unsigned long is not\n> signed, but the source type of 'int' *is* signed, and that is what\n> determines the sign extension on casting).\n>\n> So the \"is always false\" is pure and utter crap. clang is wrong, and\n> it is wrong in a way that implies that it actually generates incorrect\n> code. It may well be worth making a clang bug report about this.\n>\n> That said, clang is certainly understandably confused. The code\n> depends on subtle conversion rules and bit patterns, and is clearly\n> very confusingly written.\n>\n> So it would probably be good to rewrite it as\n>\n> unsigned long val = strtoul(line, NULL, 10);\n> if (val == ULONG_MAX) ..\n> patch->score = val;\n>\n> instead. At which point you might as well make the comparison be \">=\n> INT_MAX\" instead, since anything bigger than that is going to be\n> bogus.\n>\n> So the git code is probably worth cleaning up, but for git it would be\n> a cleanup. For clang, this implies a major bug and bad code\n> generation.\n>\n> Linus\n> Linus\n"},{"id":"207152","messageId":"20130117170209.GF4574@serenity.lan","threadId":"32650","inReplyTo":"CA+55aFxYSX2iYPSafKdCDSfWSMfQxP3R3Hqh8GuiiR6EbWfk3w@mail.gmail.com","subject":"Re: [PATCH 2/2] fix clang -Wtautological-compare with unsigned enum","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-17T17:02:09Z","receivedAt":"2013-01-17T17:02:09Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Thu, Jan 17, 2013 at 08:44:20AM -0800, Linus Torvalds wrote:\n> On Thu, Jan 17, 2013 at 3:00 AM, John Keeping wrote:\n>>\n>> There's also a warning that triggers with clang 3.2 but not clang trunk, which\n>> I think is a legitimate warning - perhaps someone who understands integer type\n>> promotion better than me can explain why the code is OK (patch->score is\n>> declared as 'int'):\n>>\n>> builtin/apply.c:1044:47: warning: comparison of constant 18446744073709551615\n>> with expression of type 'int' is always false\n>> [-Wtautological-constant-out-of-range-compare]\n>> if ((patch->score = strtoul(line, NULL, 10)) == ULONG_MAX)\n>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~\n> \n> The warning seems to be very very wrong, and implies that clang has\n> some nasty bug in it.\n> \n> Since patch->score is 'int', and UNLONG_MAX is 'unsigned long', the\n> conversion rules for the comparison is that the int result from the\n> assignment is cast to unsigned long. And if you cast (int)-1 to\n> unsigned long, you *do* get ULONG_MAX. That's true regardless of\n> whether \"long\" has the same number of bits as \"int\" or is bigger. The\n> implicit cast will be done as a sign-extension (unsigned long is not\n> signed, but the source type of 'int' *is* signed, and that is what\n> determines the sign extension on casting).\n> \n> So the \"is always false\" is pure and utter crap. clang is wrong, and\n> it is wrong in a way that implies that it actually generates incorrect\n> code. It may well be worth making a clang bug report about this.\n\nThe warning doesn't occur with a build from their trunk so it looks like\nit's already fixed - it just won't make into into a release for about 5\nmonths going by their timeline.\n\nThanks for the clear explanation.\n"},{"id":"207206","messageId":"CABURp0pj35j7+W_0gYNud2uuEoahugOMBW9ezTgPZ7YvgnBz8w@mail.gmail.com","threadId":"32650","inReplyTo":"CA+55aFxYSX2iYPSafKdCDSfWSMfQxP3R3Hqh8GuiiR6EbWfk3w@mail.gmail.com","subject":"Re: [PATCH 2/2] fix clang -Wtautological-compare with unsigned enum","fromName":"Phil Hord","fromEmail":"phil.hord@gmail.com","sentAt":"2013-01-18T17:15:05Z","receivedAt":"2013-01-18T17:15:05Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"On Thu, Jan 17, 2013 at 11:44 AM, Linus Torvalds\n wrote:\n> On Thu, Jan 17, 2013 at 3:00 AM, John Keeping wrote:\n>>\n>> There's also a warning that triggers with clang 3.2 but not clang trunk, which\n>> I think is a legitimate warning - perhaps someone who understands integer type\n>> promotion better than me can explain why the code is OK (patch->score is\n>> declared as 'int'):\n>>\n>> builtin/apply.c:1044:47: warning: comparison of constant 18446744073709551615\n>> with expression of type 'int' is always false\n>> [-Wtautological-constant-out-of-range-compare]\n>> if ((patch->score = strtoul(line, NULL, 10)) == ULONG_MAX)\n>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~\n>\n> The warning seems to be very very wrong, and implies that clang has\n> some nasty bug in it.\n>\n> Since patch->score is 'int', and UNLONG_MAX is 'unsigned long', the\n> conversion rules for the comparison is that the int result from the\n> assignment is cast to unsigned long. And if you cast (int)-1 to\n> unsigned long, you *do* get ULONG_MAX. That's true regardless of\n> whether \"long\" has the same number of bits as \"int\" or is bigger. The\n> implicit cast will be done as a sign-extension (unsigned long is not\n> signed, but the source type of 'int' *is* signed, and that is what\n> determines the sign extension on casting).\n>\n> So the \"is always false\" is pure and utter crap. clang is wrong, and\n> it is wrong in a way that implies that it actually generates incorrect\n> code. It may well be worth making a clang bug report about this.\n>\n> That said, clang is certainly understandably confused. The code\n> depends on subtle conversion rules and bit patterns, and is clearly\n> very confusingly written.\n>\n> So it would probably be good to rewrite it as\n>\n> unsigned long val = strtoul(line, NULL, 10);\n> if (val == ULONG_MAX) ..\n> patch->score = val;\n>\n> instead. At which point you might as well make the comparison be \">=\n> INT_MAX\" instead, since anything bigger than that is going to be\n> bogus.\n>\n> So the git code is probably worth cleaning up, but for git it would be\n> a cleanup. For clang, this implies a major bug and bad code\n> generation.\n\n\nYes, I can tell by the wording of the error message that you are right\nand clang has a problem. But the git code it complained about does\nhave a real problem, because the result of \"signed int a = ULONG_MAX\"\nis implementation-defined. It cannot be guaranteed or expected that\npatch->score will ever be assigned -1 there, and so the comparison may\nalways be false. I guess the warning is correct, but only\naccidentally. :-)\n\nYour rewrite is more sane and corrects the problem, I think.\n\nPhil\n"},{"id":"207210","messageId":"CA+55aFzTy0x6X_dK54RO13s+zG9ynWG_8Ei=ZwT8a5B4=LQ94A@mail.gmail.com","threadId":"32650","inReplyTo":"CABURp0pj35j7+W_0gYNud2uuEoahugOMBW9ezTgPZ7YvgnBz8w@mail.gmail.com","subject":"Re: [PATCH 2/2] fix clang -Wtautological-compare with unsigned enum","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2013-01-18T18:52:25Z","receivedAt":"2013-01-18T18:52:25Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Fri, Jan 18, 2013 at 9:15 AM, Phil Hord wrote:\n>\n> Yes, I can tell by the wording of the error message that you are right\n> and clang has a problem. But the git code it complained about does\n> have a real problem, because the result of \"signed int a = ULONG_MAX\"\n> is implementation-defined.\n\nOnly theoretically.\n\nGit won't work on machines that don't have 8-bit bytes anyway, so\nworrying about the theoretical crazy architectures that aren't two's\ncomplement etc isn't something I'd care about.\n\nThere's a whole class of \"technically implementation-defined\" issues\nin C that simply aren't worth caring for. Yes, the standard is written\nso that it works on machines that aren't byte-addressable, or EBCDIC\nor have things like 18-bit words and 36-bit longwords. Or 16-bit \"int\"\nfor microcontrollers etc.\n\nThat doesn't make those \"implementation-defined\" issues worth worrying\nabout these days. A compiler writer could in theory make up some\nidiotic rules that are still \"valid by the C standard\" even on modern\nmachines, but such a compiler should simply not be used, and the\ncompiler writer in question should be called out for being an ass-hat.\n\nPaper standards are only worth so much. And that \"so much\" really\nisn't very much.\n\n Linus\n"},{"id":"208446","messageId":"87fw1gwq4o.fsf@catnip.gol.com","threadId":"32650","inReplyTo":"20130116175057.GB27525@sigill.intra.peff.net","subject":"Re: [PATCH] fix some clang warnings","fromName":"Miles Bader","fromEmail":"miles@gnu.org","sentAt":"2013-02-01T05:37:59Z","receivedAt":"2013-02-01T05:37:59Z","isPatch":true,"sender":{"key":"miles@gnu.org","avatar":"https://gravatar.com/avatar/01069b69593af7bff28e2f97afeb3644ae6fe2f5f56cb3a8cf34c5fb8c36efe5?d=mp&s=160"},"body":"Jeff King writes:\n> It seems a little weird to me that clang defines __GNUC__, but I\n> assume there are good reasons for it.\n\nThe thing is that \"gcc\" is as much a language dialect these days as it\nis a compiler implementation, and many other compilers, including\nclang, explicitly try to implement that dialect (clang goes even\nfurther by trying to be compatible in other ways, e.g. command-line\nsyntax, but that's not relevant here).\n\n__GNUC__ is a way many programs try to detect the presence of a\ncompiler that implements that dialect, they have little choice but to\ndefine it...\n\n-Miles\n\n-- \nLove, n. A temporary insanity curable by marriage or by removal of the patient\nfrom the influences under which he incurred the disorder. This disease is\nprevalent only among civilized races living under artificial conditions;\nbarbarous nations breathing pure air and eating simple food enjoy immunity\nfrom its ravages. It is sometimes fatal, but more frequently to the physician\nthan to the patient.\n"}]}