Re: [PATCH] fix some clang warnings
Jeff King p...@peff.net writes: It seems a little weird to me that clang defines __GNUC__, but I assume there are good reasons for it. The thing is that gcc is as much a language dialect these days as it is a compiler implementation, and many other compilers, including clang, explicitly try to implement that dialect (clang goes even further by trying to be compatible in other ways, e.g. command-line syntax, but that's not relevant here). __GNUC__ is a way many programs try to detect the presence of a compiler that implements that dialect, they have little choice but to define it... -Miles -- Love, n. A temporary insanity curable by marriage or by removal of the patient from the influences under which he incurred the disorder. This disease is prevalent only among civilized races living under artificial conditions; barbarous nations breathing pure air and eating simple food enjoy immunity from its ravages. It is sometimes fatal, but more frequently to the physician than to the patient. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fix some clang warnings
On Wed, Jan 16, 2013 at 07:01:37PM +, John Keeping wrote: On Wed, Jan 16, 2013 at 10:24:49AM -0800, Jeff King wrote: On Wed, Jan 16, 2013 at 06:22:40PM +, John Keeping wrote: Thanks for checking. I'd rather squelch the warning completely (as in my re-post of Max's patch from a few minutes ago), and we can loosen it (possibly with a version check) later when a fix is widely disseminated. I checked again with a trunk build of clang and the warning's still there, so I've created a clang bug [1] to see if they will change the behaviour. [1] http://llvm.org/bugs/show_bug.cgi?id=14968 Well, that was quick! This warning is now gone when using a fresh trunk build of clang. From [2], it looks like this will become version 3.3 (in about 5 months). So should we change the condition to: #if defined(__GNUC__) (!defined(__clang__) || __clang_major__ 3 || \ (__clang__major == 3 __clang_minor__ = 3) [2] http://llvm.org/docs/HowToReleaseLLVM.html John -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] fix some clang warnings
Signed-off-by: Max Horn m...@quendi.de --- cache.h | 2 +- git-compat-util.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cache.h b/cache.h index c257953..5c8440b 100644 --- a/cache.h +++ b/cache.h @@ -1148,7 +1148,7 @@ extern int check_repository_format_version(const char *var, const char *value, v extern int git_env_bool(const char *, int); extern int git_config_system(void); extern int config_error_nonbool(const char *); -#ifdef __GNUC__ +#if defined(__GNUC__) ! defined(__clang__) #define config_error_nonbool(s) (config_error_nonbool(s), -1) #endif extern const char *get_log_output_encoding(void); diff --git a/git-compat-util.h b/git-compat-util.h index 4f022a3..cc2abee 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -310,7 +310,7 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2))) * 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. */ -#ifdef __GNUC__ +#if defined(__GNUC__) ! defined(__clang__) #define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1) #endif -- 1.8.1.1.435.g4e2ebdf -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fix some clang warnings
On Wed, Jan 16, 2013 at 03:53:23PM +0100, Max Horn wrote: -#ifdef __GNUC__ +#if defined(__GNUC__) ! defined(__clang__) #define config_error_nonbool(s) (config_error_nonbool(s), -1) #endif You don't say what the warning is, but I'm guessing it's complaining about throwing away the return value from config_error_nonbool? -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fix some clang warnings
Jeff King p...@peff.net writes: On Wed, Jan 16, 2013 at 03:53:23PM +0100, Max Horn wrote: -#ifdef __GNUC__ +#if defined(__GNUC__) ! defined(__clang__) #define config_error_nonbool(s) (config_error_nonbool(s), -1) #endif You don't say what the warning is, but I'm guessing it's complaining about throwing away the return value from config_error_nonbool? Yeah, I was wondering about the same thing. The other one looks similar, ignoring the return value of error(). Also, is this some versions of clang do not like this? Or are all versions of clang affected? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fix some clang warnings
FWIW, I also happen to have the warning:
advice.c:69:2: warning: expression result unused [-Wunused-value]
error('%s' is not possible because you have unmerged files., me);
^~
./git-compat-util.h:314:55: note: expanded from:
#define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)
^~
with clang: Ubuntu clang version 3.0-6ubuntu3 (tags/RELEASE_30/final)
(based on LLVM 3.0)
I can't say about other versions.
On Wed, Jan 16, 2013 at 5:53 PM, Junio C Hamano gits...@pobox.com wrote:
Jeff King p...@peff.net writes:
On Wed, Jan 16, 2013 at 03:53:23PM +0100, Max Horn wrote:
-#ifdef __GNUC__
+#if defined(__GNUC__) ! defined(__clang__)
#define config_error_nonbool(s) (config_error_nonbool(s), -1)
#endif
You don't say what the warning is, but I'm guessing it's complaining
about throwing away the return value from config_error_nonbool?
Yeah, I was wondering about the same thing. The other one looks
similar, ignoring the return value of error().
Also, is this some versions of clang do not like this? Or are all
versions of clang affected?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fix some clang warnings
On Wed, Jan 16, 2013 at 06:12:57PM +0100, Antoine Pelisse wrote:
FWIW, I also happen to have the warning:
advice.c:69:2: warning: expression result unused [-Wunused-value]
error('%s' is not possible because you have unmerged files., me);
^~
./git-compat-util.h:314:55: note: expanded from:
#define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)
^~
with clang: Ubuntu clang version 3.0-6ubuntu3 (tags/RELEASE_30/final)
(based on LLVM 3.0)
I have the same output with:
clang version 3.2 (tags/RELEASE_32/final)
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fix some clang warnings
On 16.01.2013, at 18:18, John Keeping wrote:
On Wed, Jan 16, 2013 at 06:12:57PM +0100, Antoine Pelisse wrote:
FWIW, I also happen to have the warning:
advice.c:69:2: warning: expression result unused [-Wunused-value]
error('%s' is not possible because you have unmerged files., me);
^~
./git-compat-util.h:314:55: note: expanded from:
#define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)
^~
with clang: Ubuntu clang version 3.0-6ubuntu3 (tags/RELEASE_30/final)
(based on LLVM 3.0)
I have the same output with:
clang version 3.2 (tags/RELEASE_32/final)
Sorry for not being more specific in my message. I have this with
Apple clang version 4.1 (tags/Apple/clang-421.11.66) (based on LLVM 3.1svn)
Max
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fix some clang warnings
On Wed, Jan 16, 2013 at 06:26:35PM +0100, Max Horn wrote:
On Wed, Jan 16, 2013 at 06:12:57PM +0100, Antoine Pelisse wrote:
FWIW, I also happen to have the warning:
advice.c:69:2: warning: expression result unused [-Wunused-value]
error('%s' is not possible because you have unmerged files., me);
^~
./git-compat-util.h:314:55: note: expanded from:
#define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)
^~
with clang: Ubuntu clang version 3.0-6ubuntu3 (tags/RELEASE_30/final)
(based on LLVM 3.0)
I have the same output with:
clang version 3.2 (tags/RELEASE_32/final)
Sorry for not being more specific in my message. I have this with
Apple clang version 4.1 (tags/Apple/clang-421.11.66) (based on LLVM 3.1svn)
So it seems pretty common, and is just that clang is more concerned
about this than gcc. I think your patch is a reasonable workaround. It
seems a little weird to me that clang defines __GNUC__, but I assume
there are good reasons for it. The commit message should probably be
along the lines of:
Commit a469a10 wraps some error calls in macros to give the compiler a
chance to do more static analysis on their constant -1 return value.
We limit the use of these macros to __GNUC__, since gcc is the primary
beneficiary of the new information, and because we use GNU features
for handling variadic macros.
However, clang also defines __GNUC__, but generates warnings (due to
throwing away the return value from the first half of the macro). We
can squelch the warning by turning off these macros when clang is in
use.
I'm confused, though, why your patch does not have a matching update to
the opterror macro in parse-options.h. It uses exactly the same
technique. Does it not generate a warning?
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fix some clang warnings
On Wed, Jan 16, 2013 at 09:50:57AM -0800, Jeff King wrote:
I'm confused, though, why your patch does not have a matching update to
the opterror macro in parse-options.h. It uses exactly the same
technique. Does it not generate a warning?
Ah, I think I see why not.
It is not about the macro itself, but rather the callsites that do not
return error, but call it for its printing side effect. It seems that
clang -Wunused-value is OK with unused values from functions being
discarded, but not with constants. So:
int foo();
void bar()
{
foo(); /* ok */
1; /* not ok */
(foo(), 1); /* not ok */
}
The first one is OK (I think it would fall under -Wunused-result under
either compiler). The middle one is an obvious error, and caught by both
compilers. The last one is OK by gcc, but clang complains.
So opterror does not happen to generate any warnings, because we do not
ever use it in a void context. It should probably be marked the same
way, though, as future-proofing.
The commit message should probably be along the lines of:
[...]
However, clang also defines __GNUC__, but generates warnings (due to
throwing away the return value from the first half of the macro). We
can squelch the warning by turning off these macros when clang is in
use.
So a more accurate description would be:
However, clang also defines __GNUC__, but generates warnings with
-Wunused-value when these macros are used in a void context, because
the constant -1 ends up being useless. Gcc does not complain about
this case (though it is unclear if it is because it is smart enough to
see what we are doing, or too dumb to realize that the -1 is unused).
We can squelch the warning by just disabling these macros when clang
is in use.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fix some clang warnings
On Wed, 16 Jan 2013 09:50:57 -0800, Jeff King p...@peff.net wrote: However, clang also defines __GNUC__, [...] http://sourceforge.net/p/predef/wiki/Compilers/ Notice that the meaning of the __GNUC__ macro has changed subtly over the years, from identifying the GNU C/C++ compiler to identifying any compiler that implements the GNU compiler extensions (...). For example, the Intel C++ on Linux also defines these macros from version 8.1 (...). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fix some clang warnings
On Wed, Jan 16, 2013 at 10:00:42AM -0800, Jeff King wrote: So opterror does not happen to generate any warnings, because we do not ever use it in a void context. It should probably be marked the same way, though, as future-proofing. [...] So a more accurate description would be: Here it is all together: -- 8 -- From: Max Horn m...@quendi.de Subject: [PATCH] fix clang -Wunused-value warnings for error functions Commit a469a10 wraps some error calls in macros to give the compiler a chance to do more static analysis on their constant -1 return value. We limit the use of these macros to __GNUC__, since gcc is the primary beneficiary of the new information, and because we use GNU features for handling variadic macros. However, clang also defines __GNUC__, but generates warnings with -Wunused-value when these macros are used in a void context, because the constant -1 ends up being useless. Gcc does not complain about this case (though it is unclear if it is because it is smart enough to see what we are doing, or too dumb to realize that the -1 is unused). We can squelch the warning by just disabling these macros when clang is in use. Signed-off-by: Max Horn m...@quendi.de Signed-off-by: Jeff King p...@peff.net --- cache.h | 2 +- git-compat-util.h | 2 +- parse-options.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cache.h b/cache.h index c257953..5c8440b 100644 --- a/cache.h +++ b/cache.h @@ -1148,7 +1148,7 @@ extern int config_error_nonbool(const char *); extern int git_env_bool(const char *, int); extern int git_config_system(void); extern int config_error_nonbool(const char *); -#ifdef __GNUC__ +#if defined(__GNUC__) ! defined(__clang__) #define config_error_nonbool(s) (config_error_nonbool(s), -1) #endif extern const char *get_log_output_encoding(void); diff --git a/git-compat-util.h b/git-compat-util.h index 2cecf56..2596280 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -297,7 +297,7 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2))) * 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. */ -#ifdef __GNUC__ +#if defined(__GNUC__) ! defined(__clang__) #define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1) #endif diff --git a/parse-options.h b/parse-options.h index e703853..1c8bd8d 100644 --- a/parse-options.h +++ b/parse-options.h @@ -177,7 +177,7 @@ extern int opterror(const struct option *opt, const char *reason, int flags); extern int optbug(const struct option *opt, const char *reason); extern int opterror(const struct option *opt, const char *reason, int flags); -#ifdef __GNUC__ +#if defined(__GNUC__) ! defined(clang) #define opterror(o,r,f) (opterror((o),(r),(f)), -1) #endif -- 1.8.1.rc1.10.g7d71f7b -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fix some clang warnings
On Wed, Jan 16, 2013 at 10:00:42AM -0800, Jeff King wrote:
It is not about the macro itself, but rather the callsites that do not
return error, but call it for its printing side effect. It seems that
clang -Wunused-value is OK with unused values from functions being
discarded, but not with constants. So:
int foo();
void bar()
{
foo(); /* ok */
1; /* not ok */
(foo(), 1); /* not ok */
}
The first one is OK (I think it would fall under -Wunused-result under
either compiler). The middle one is an obvious error, and caught by both
compilers. The last one is OK by gcc, but clang complains.
I wonder if this would be changed in clang - the change in [1] is
superficially similar.
[1] http://llvm.org/bugs/show_bug.cgi?id=13747
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fix some clang warnings
Jeff King p...@peff.net writes: It seems a little weird to me that clang defines __GNUC__, but I assume there are good reasons for it. The reason is essentially that clang targets compatibility with GCC (implementing the same extensions cie), in the sense drop in replacement that should be able to compile legacy code possibly relying on __GNUC__ and GCC extensions. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fix some clang warnings
On Wed, Jan 16, 2013 at 06:12:03PM +, John Keeping wrote:
On Wed, Jan 16, 2013 at 10:00:42AM -0800, Jeff King wrote:
It is not about the macro itself, but rather the callsites that do not
return error, but call it for its printing side effect. It seems that
clang -Wunused-value is OK with unused values from functions being
discarded, but not with constants. So:
int foo();
void bar()
{
foo(); /* ok */
1; /* not ok */
(foo(), 1); /* not ok */
}
The first one is OK (I think it would fall under -Wunused-result under
either compiler). The middle one is an obvious error, and caught by both
compilers. The last one is OK by gcc, but clang complains.
I wonder if this would be changed in clang - the change in [1] is
superficially similar.
[1] http://llvm.org/bugs/show_bug.cgi?id=13747
Yeah, I think it is exactly the same issue, and the fix they mention
there would apply to us, too.
Is it worth applying this at all, then? Or should we apply it but limit
it with a clang version macro (they mention r163034, but I do not know
if it is in a released version yet, nor what macros are available to
inspect the version)?
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fix some clang warnings
Is it worth applying this at all, then? Or should we apply it but limit it with a clang version macro (they mention r163034, but I do not know if it is in a released version yet, nor what macros are available to inspect the version)? Please also note that building with clang is not warning-free (though I think it would be nice) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fix some clang warnings
On Wed, Jan 16, 2013 at 10:15:58AM -0800, Jeff King wrote:
On Wed, Jan 16, 2013 at 06:12:03PM +, John Keeping wrote:
On Wed, Jan 16, 2013 at 10:00:42AM -0800, Jeff King wrote:
It is not about the macro itself, but rather the callsites that do not
return error, but call it for its printing side effect. It seems that
clang -Wunused-value is OK with unused values from functions being
discarded, but not with constants. So:
int foo();
void bar()
{
foo(); /* ok */
1; /* not ok */
(foo(), 1); /* not ok */
}
The first one is OK (I think it would fall under -Wunused-result under
either compiler). The middle one is an obvious error, and caught by both
compilers. The last one is OK by gcc, but clang complains.
I wonder if this would be changed in clang - the change in [1] is
superficially similar.
[1] http://llvm.org/bugs/show_bug.cgi?id=13747
Yeah, I think it is exactly the same issue, and the fix they mention
there would apply to us, too.
Is it worth applying this at all, then? Or should we apply it but limit
it with a clang version macro (they mention r163034, but I do not know
if it is in a released version yet, nor what macros are available to
inspect the version)?
That maps to revision 06b3a06007 in their git repository [1], which is
contained in remotes/origin/release_32 so I think that change should be
in release 3.2, where I still see the warning (although that's not using
a clang built from that source), so I don't think that the fix for that
bug removes the warning in this case.
[1] http://llvm.org/git/clang.git
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fix some clang warnings
On Wed, Jan 16, 2013 at 10:24:49AM -0800, Jeff King wrote: On Wed, Jan 16, 2013 at 06:22:40PM +, John Keeping wrote: [1] http://llvm.org/bugs/show_bug.cgi?id=13747 Yeah, I think it is exactly the same issue, and the fix they mention there would apply to us, too. Is it worth applying this at all, then? Or should we apply it but limit it with a clang version macro (they mention r163034, but I do not know if it is in a released version yet, nor what macros are available to inspect the version)? That maps to revision 06b3a06007 in their git repository [1], which is contained in remotes/origin/release_32 so I think that change should be in release 3.2, where I still see the warning (although that's not using a clang built from that source), so I don't think that the fix for that bug removes the warning in this case. [1] http://llvm.org/git/clang.git Thanks for checking. I'd rather squelch the warning completely (as in my re-post of Max's patch from a few minutes ago), and we can loosen it (possibly with a version check) later when a fix is widely disseminated. I checked again with a trunk build of clang and the warning's still there, so I've created a clang bug [1] to see if they will change the behaviour. I agree that we should squelch the warning for now, it can be changed into a version check if it's accepted as a bug and once we know what version it's fixed in. [1] http://llvm.org/bugs/show_bug.cgi?id=14968 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
