Re: [PATCH] fix some clang warnings

2013-01-31 Thread Miles Bader
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

Re: [PATCH] fix some clang warnings

2013-01-17 Thread John Keeping
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

[PATCH] fix some clang warnings

2013-01-16 Thread Max Horn
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

Re: [PATCH] fix some clang warnings

2013-01-16 Thread Jeff King
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

Re: [PATCH] fix some clang warnings

2013-01-16 Thread Junio C Hamano
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

Re: [PATCH] fix some clang warnings

2013-01-16 Thread Antoine Pelisse
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:

Re: [PATCH] fix some clang warnings

2013-01-16 Thread John Keeping
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);

Re: [PATCH] fix some clang warnings

2013-01-16 Thread Max Horn
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);

Re: [PATCH] fix some clang warnings

2013-01-16 Thread Jeff King
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

Re: [PATCH] fix some clang warnings

2013-01-16 Thread Jeff King
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

Re: [PATCH] fix some clang warnings

2013-01-16 Thread Tomas Carnecky
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

Re: [PATCH] fix some clang warnings

2013-01-16 Thread Jeff King
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

Re: [PATCH] fix some clang warnings

2013-01-16 Thread John Keeping
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

Re: [PATCH] fix some clang warnings

2013-01-16 Thread Matthieu Moy
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

Re: [PATCH] fix some clang warnings

2013-01-16 Thread Jeff King
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

Re: [PATCH] fix some clang warnings

2013-01-16 Thread Antoine Pelisse
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

Re: [PATCH] fix some clang warnings

2013-01-16 Thread John Keeping
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

Re: [PATCH] fix some clang warnings

2013-01-16 Thread John Keeping
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