[PATCH] fix some clang warnings

[PATCH] fix some clang warnings

[Max Horn]

Signed-off-by: Max Horn <max@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

Re: [PATCH] fix some clang warnings

[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 value from config_error_nonbool?

-Peff

Re: [PATCH] fix some clang warnings

[Junio C Hamano]

Jeff King <peff@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?
 

Re: [PATCH] fix some clang warnings

[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: 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 <gitster@pobox.com> wrote:
> Jeff King <peff@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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fix some clang warnings

[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);
>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ./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)

Re: [PATCH] fix some clang warnings

[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);
>>        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> ./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

Re: [PATCH] fix some clang warnings

[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 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

Re: [PATCH] fix some clang warnings

[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 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

Re: [PATCH] fix some clang warnings

[Tomas Carnecky]

On Wed, 16 Jan 2013 09:50:57 -0800, Jeff King <peff@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 (...).

Re: [PATCH] fix some clang warnings

[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 all together:

-- >8 --
From: Max Horn <max@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 <max@quendi.de>
Signed-off-by: Jeff King <peff@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

Re: [PATCH] fix some clang warnings

[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 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

Re: [PATCH] fix some clang warnings

[Matthieu Moy]

Jeff King <peff@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/

Re: [PATCH] fix some clang warnings

[Jeff King]

On Wed, Jan 16, 2013 at 06:12:03PM +0000, 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

Re: [PATCH] fix some clang warnings

[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 warning-free (though
I think it would be nice)

Re: [PATCH] fix some clang warnings

[John Keeping]

On Wed, Jan 16, 2013 at 10:15:58AM -0800, Jeff King wrote:
> On Wed, Jan 16, 2013 at 06:12:03PM +0000, 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

Re: [PATCH] fix some clang warnings

[Jeff King]

On Wed, Jan 16, 2013 at 06:22:40PM +0000, 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 know that compiling git with clang is not warning-free yet, but it is
close, and I do not mind if somebody puts some effort into making it so.
This gets us one step closer.

-Peff

Re: [PATCH] fix some clang warnings

[John Keeping]

On Wed, Jan 16, 2013 at 10:24:49AM -0800, Jeff King wrote:
> On Wed, Jan 16, 2013 at 06:22:40PM +0000, 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

[PATCH 1/2] fix clang -Wconstant-conversion with bit fields

[Antoine Pelisse]

clang incorrectly reports a constant conversion warning (implicit
truncation to bit field) when using the "flag &= ~FLAG" form, because
~FLAG needs to be truncated.

Convert this form to "flag = flag & ~FLAG" fixes the issue as
the right operand now fits into the bit field.

Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
---
I'm sorry about this fix, it really seems bad, yet it's one step closer
to warning-free clang compilation.

It seems quite clear to me that it's a bug in clang.

 bisect.c           | 2 +-
 builtin/checkout.c | 2 +-
 builtin/reflog.c   | 4 ++--
 commit.c           | 4 ++--
 revision.c         | 8 ++++----
 upload-pack.c      | 4 ++--
 6 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/bisect.c b/bisect.c
index bd1b7b5..34ac01d 100644
--- a/bisect.c
+++ b/bisect.c
@@ -63,7 +63,7 @@ static void clear_distance(struct commit_list *list)
 {
 	while (list) {
 		struct commit *commit = list->item;
-		commit->object.flags &= ~COUNTED;
+		commit->object.flags = commit->object.flags & ~COUNTED;
 		list = list->next;
 	}
 }
diff --git a/builtin/checkout.c b/builtin/checkout.c
index a9c1b5a..2c83234 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -717,7 +717,7 @@ static void orphaned_commit_warning(struct commit *old, struct commit *new)
 	init_revisions(&revs, NULL);
 	setup_revisions(0, NULL, &revs, NULL);

-	object->flags &= ~UNINTERESTING;
+	object->flags = object->flags & ~UNINTERESTING;
 	add_pending_object(&revs, object, sha1_to_hex(object->sha1));

 	for_each_ref(add_pending_uninteresting_ref, &revs);
diff --git a/builtin/reflog.c b/builtin/reflog.c
index b3c9e27..3079c81 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -170,7 +170,7 @@ static int commit_is_complete(struct commit *commit)
 	}
 	/* clear flags from the objects we traversed */
 	for (i = 0; i < found.nr; i++)
-		found.objects[i].item->flags &= ~STUDYING;
+		found.objects[i].item->flags = found.objects[i].item->flags&  ~STUDYING;
 	if (is_incomplete)
 		commit->object.flags |= INCOMPLETE;
 	else {
@@ -229,7 +229,7 @@ static void mark_reachable(struct expire_reflog_cb *cb)
 	struct commit_list *leftover = NULL;

 	for (pending = cb->mark_list; pending; pending = pending->next)
-		pending->item->object.flags &= ~REACHABLE;
+		pending->item->object.flags = pending->item->object.flags & ~REACHABLE;

 	pending = cb->mark_list;
 	while (pending) {
diff --git a/commit.c b/commit.c
index e8eb0ae..800779d 100644
--- a/commit.c
+++ b/commit.c
@@ -883,7 +883,7 @@ struct commit_list *reduce_heads(struct commit_list *heads)

 	/* Uniquify */
 	for (p = heads; p; p = p->next)
-		p->item->object.flags &= ~STALE;
+		p->item->object.flags = p->item->object.flags & ~STALE;
 	for (p = heads, num_head = 0; p; p = p->next) {
 		if (p->item->object.flags & STALE)
 			continue;
@@ -894,7 +894,7 @@ struct commit_list *reduce_heads(struct commit_list *heads)
 	for (p = heads, i = 0; p; p = p->next) {
 		if (p->item->object.flags & STALE) {
 			array[i++] = p->item;
-			p->item->object.flags &= ~STALE;
+			p->item->object.flags = p->item->object.flags & ~STALE;
 		}
 	}
 	num_head = remove_redundant(array, num_head);
diff --git a/revision.c b/revision.c
index d7562ee..ed1c16d 100644
--- a/revision.c
+++ b/revision.c
@@ -787,9 +787,9 @@ static void limit_to_ancestry(struct commit_list *bottom, struct commit_list *li

 	/* We are done with the TMP_MARK */
 	for (p = list; p; p = p->next)
-		p->item->object.flags &= ~TMP_MARK;
+		p->item->object.flags = p->item->object.flags & ~TMP_MARK;
 	for (p = bottom; p; p = p->next)
-		p->item->object.flags &= ~TMP_MARK;
+		p->item->object.flags = p->item->object.flags & ~TMP_MARK;
 	free_commit_list(rlist);
 }

@@ -1948,7 +1948,7 @@ static int remove_duplicate_parents(struct commit *commit)
 	/* count them while clearing the temporary mark */
 	surviving_parents = 0;
 	for (p = commit->parents; p; p = p->next) {
-		p->item->object.flags &= ~TMP_MARK;
+		p->item->object.flags = p->item->object.flags & ~TMP_MARK;
 		surviving_parents++;
 	}
 	return surviving_parents;
@@ -2378,7 +2378,7 @@ static struct commit *get_revision_1(struct rev_info *revs)

 		if (revs->reflog_info) {
 			fake_reflog_parent(revs->reflog_info, commit);
-			commit->object.flags &= ~(ADDED | SEEN | SHOWN);
+			commit->object.flags = commit->object.flags & ~(ADDED | SEEN | SHOWN);
 		}

 		/*
diff --git a/upload-pack.c b/upload-pack.c
index 7c05b15..74d8f0e 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -113,7 +113,7 @@ static int do_rev_list(int in, int out, void *user_data)
 	for (i = 0; i < want_obj.nr; i++) {
 		struct object *o = want_obj.objects[i].item;
 		/* why??? */
-		o->flags &= ~UNINTERESTING;
+		o->flags = o->flags & ~UNINTERESTING;
 		add_pending_object(&revs, o, NULL);
 	}
 	for (i = 0; i < have_obj.nr; i++) {
@@ -700,7 +700,7 @@ static void receive_needs(void)
 				struct commit_list *parents;
 				packet_write(1, "unshallow %s",
 					sha1_to_hex(object->sha1));
-				object->flags &= ~CLIENT_SHALLOW;
+				object->flags = object->flags & ~CLIENT_SHALLOW;
 				/* make sure the real parents are parsed */
 				unregister_shallow(object->sha1);
 				object->parsed = 0;
--
1.8.1.1.435.g20d29be.dirty

[PATCH 2/2] fix clang -Wtautological-compare with unsigned enum

[Antoine Pelisse]

Create a GREP_HEADER_FIELD_MIN so we can check that the field value is
sane and silent the clang warning.

Clang warning happens because the enum is unsigned (this is
implementation-defined, and there is no negative fields) and the check
is then tautological.

Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
---
I tried to consider discussion [1] and this [2] discussion on clang's list

With these two patches and the patch from Max Horne, I'm finally able to
compile with CC=clang CFLAGS=-Werror.

 [1]: http://thread.gmane.org/gmane.comp.version-control.git/184908
 [2]: http://clang-developers.42468.n3.nabble.com/Possibly-invalid-enum-tautology-warning-td3233140.html

 grep.c | 3 ++-
 grep.h | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/grep.c b/grep.c
index 4bd1b8b..bb548ca 100644
--- a/grep.c
+++ b/grep.c
@@ -625,7 +625,8 @@ static struct grep_expr *prep_header_patterns(struct grep_opt *opt)
 	for (p = opt->header_list; p; p = p->next) {
 		if (p->token != GREP_PATTERN_HEAD)
 			die("bug: a non-header pattern in grep header list.");
-		if (p->field < 0 || GREP_HEADER_FIELD_MAX <= p->field)
+		if (p->field < GREP_HEADER_FIELD_MIN ||
+		    GREP_HEADER_FIELD_MAX <= p->field)
 			die("bug: unknown header field %d", p->field);
 		compile_regexp(p, opt);
 	}
diff --git a/grep.h b/grep.h
index 8fc854f..e4a1df5 100644
--- a/grep.h
+++ b/grep.h
@@ -28,7 +28,8 @@ enum grep_context {
 };

 enum grep_header_field {
-	GREP_HEADER_AUTHOR = 0,
+	GREP_HEADER_FIELD_MIN = 0,
+	GREP_HEADER_AUTHOR = GREP_HEADER_FIELD_MIN,
 	GREP_HEADER_COMMITTER,
 	GREP_HEADER_REFLOG,

--
1.8.1.1.435.g20d29be.dirty

Re: [PATCH 1/2] fix clang -Wconstant-conversion with bit fields

[John Keeping]

On Wed, Jan 16, 2013 at 11:47:22PM +0100, Antoine Pelisse wrote:
> clang incorrectly reports a constant conversion warning (implicit
> truncation to bit field) when using the "flag &= ~FLAG" form, because
> ~FLAG needs to be truncated.
> 
> Convert this form to "flag = flag & ~FLAG" fixes the issue as
> the right operand now fits into the bit field.
> 
> Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
> ---
> I'm sorry about this fix, it really seems bad, yet it's one step closer
> to warning-free clang compilation.
> 
> It seems quite clear to me that it's a bug in clang.

Which version of clang did you see this with?  I don't get these
warnings with clang 3.2.


John

Re: [PATCH 1/2] fix clang -Wconstant-conversion with bit fields

[Antoine Pelisse]

On Thu, Jan 17, 2013 at 12:08 AM, John Keeping <john@keeping.me.uk> wrote:
> On Wed, Jan 16, 2013 at 11:47:22PM +0100, Antoine Pelisse wrote:
>> clang incorrectly reports a constant conversion warning (implicit
>> truncation to bit field) when using the "flag &= ~FLAG" form, because
>> ~FLAG needs to be truncated.
> Which version of clang did you see this with?  I don't get these
> warnings with clang 3.2.

Ubuntu clang version 3.0-6ubuntu3 (tags/RELEASE_30/final) (based on LLVM 3.0)

It's good to know it's been fixed !

Re: [PATCH 2/2] fix clang -Wtautological-compare with unsigned enum

[Antoine Pelisse]

> With these two patches and the patch from Max Horne,

I'm deeply sorry for this typo Max

Re: [PATCH 1/2] fix clang -Wconstant-conversion with bit fields

[Antoine Pelisse]

So I guess we should drop this patch, it's probably not worth it,
especially if it's been fixed already by clang.

On Thu, Jan 17, 2013 at 12:09 AM, Antoine Pelisse <apelisse@gmail.com> wrote:
> On Thu, Jan 17, 2013 at 12:08 AM, John Keeping <john@keeping.me.uk> wrote:
>> On Wed, Jan 16, 2013 at 11:47:22PM +0100, Antoine Pelisse wrote:
>>> clang incorrectly reports a constant conversion warning (implicit
>>> truncation to bit field) when using the "flag &= ~FLAG" form, because
>>> ~FLAG needs to be truncated.
>> Which version of clang did you see this with?  I don't get these
>> warnings with clang 3.2.
>
> Ubuntu clang version 3.0-6ubuntu3 (tags/RELEASE_30/final) (based on LLVM 3.0)
>
> It's good to know it's been fixed !

Re: [PATCH 1/2] fix clang -Wconstant-conversion with bit fields

[Junio C Hamano]

Antoine Pelisse <apelisse@gmail.com> writes:

> clang incorrectly reports a constant conversion warning (implicit
> truncation to bit field) when using the "flag &= ~FLAG" form, because
> ~FLAG needs to be truncated.
>
> Convert this form to "flag = flag & ~FLAG" fixes the issue as
> the right operand now fits into the bit field.

If the "clang incorrectly reports" is already recognised by clang
folks as a bug to be fixed in clang, I'd rather not to take this
patch.

I do not think it is reasonable to expect people to remember that
they have to write "flags &= ~TO_DROP" in a longhand whenever they
are adding new code that needs to do bit-fields, so even if this
patch makes clang silent for the _current_ code, it will not stay
that way.  Something like

#define FLIP_BIT_CLR(fld,bit) do { \
	typeof(fld) *x = &(fld); \
        *x = *x & (~(bit)); \
} while (0)

may be more palapable but not by a large margin.

Yuck.

Re: [PATCH 1/2] fix clang -Wconstant-conversion with bit fields

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Antoine Pelisse <apelisse@gmail.com> writes:
>
>> clang incorrectly reports a constant conversion warning (implicit
>> truncation to bit field) when using the "flag &= ~FLAG" form, because
>> ~FLAG needs to be truncated.
>>
>> Convert this form to "flag = flag & ~FLAG" fixes the issue as
>> the right operand now fits into the bit field.
>
> If the "clang incorrectly reports" is already recognised by clang
> folks as a bug to be fixed in clang, I'd rather not to take this
> patch.
>
> I do not think it is reasonable to expect people to remember that
> they have to write "flags &= ~TO_DROP" in a longhand whenever they
> are adding new code that needs to do bit-fields, so even if this
> patch makes clang silent for the _current_ code, it will not stay
> that way.  Something like
>
> #define FLIP_BIT_CLR(fld,bit) do { \
> 	typeof(fld) *x = &(fld); \
>         *x = *x & (~(bit)); \
> } while (0)
>
> may be more palapable but not by a large margin.
>
> Yuck.

Double yuck.  I meant palatable.

In any case, I see somebody reports that more recent clang does not
have this bug in the near-by message, so let's forget about this
issue.

Thanks.

Re: [PATCH] fix some clang warnings

[John Keeping]

On Wed, Jan 16, 2013 at 07:01:37PM +0000, 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 +0000, 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

Re: [PATCH 2/2] fix clang -Wtautological-compare with unsigned enum

[Antoine Pelisse]

John, could you confirm that you trigger the -Wtautological-compare
warning with your version of clang ?
And that this patch makes clang compilation warning-free (with the
very latest clang) ?

Cheers,

On Wed, Jan 16, 2013 at 11:47 PM, Antoine Pelisse <apelisse@gmail.com> wrote:
> Create a GREP_HEADER_FIELD_MIN so we can check that the field value is
> sane and silent the clang warning.
>
> Clang warning happens because the enum is unsigned (this is
> implementation-defined, and there is no negative fields) and the check
> is then tautological.
>
> Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
> ---
> I tried to consider discussion [1] and this [2] discussion on clang's list
>
> With these two patches and the patch from Max Horne, I'm finally able to
> compile with CC=clang CFLAGS=-Werror.
>
>  [1]: http://thread.gmane.org/gmane.comp.version-control.git/184908
>  [2]: http://clang-developers.42468.n3.nabble.com/Possibly-invalid-enum-tautology-warning-td3233140.html
>
>  grep.c | 3 ++-
>  grep.h | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/grep.c b/grep.c
> index 4bd1b8b..bb548ca 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -625,7 +625,8 @@ static struct grep_expr *prep_header_patterns(struct grep_opt *opt)
>         for (p = opt->header_list; p; p = p->next) {
>                 if (p->token != GREP_PATTERN_HEAD)
>                         die("bug: a non-header pattern in grep header list.");
> -               if (p->field < 0 || GREP_HEADER_FIELD_MAX <= p->field)
> +               if (p->field < GREP_HEADER_FIELD_MIN ||
> +                   GREP_HEADER_FIELD_MAX <= p->field)
>                         die("bug: unknown header field %d", p->field);
>                 compile_regexp(p, opt);
>         }
> diff --git a/grep.h b/grep.h
> index 8fc854f..e4a1df5 100644
> --- a/grep.h
> +++ b/grep.h
> @@ -28,7 +28,8 @@ enum grep_context {
>  };
>
>  enum grep_header_field {
> -       GREP_HEADER_AUTHOR = 0,
> +       GREP_HEADER_FIELD_MIN = 0,
> +       GREP_HEADER_AUTHOR = GREP_HEADER_FIELD_MIN,
>         GREP_HEADER_COMMITTER,
>         GREP_HEADER_REFLOG,
>
> --
> 1.8.1.1.435.g20d29be.dirty
>

Re: [PATCH 2/2] fix clang -Wtautological-compare with unsigned enum

[John Keeping]

On Thu, Jan 17, 2013 at 11:32:39AM +0100, Antoine Pelisse wrote:
> John, could you confirm that you trigger the -Wtautological-compare
> warning with your version of clang ?

Yes, the warning is still there with both 3.2 and a recent trunk build
but this patch squelches it.

> And that this patch makes clang compilation warning-free (with the
> very latest clang) ?

There is one remaining warning on pu which hasn't been discussed in this
thread as far as I can see.  I'll send a patch shortly.

There's also a warning that triggers with clang 3.2 but not clang trunk, which
I think is a legitimate warning - perhaps someone who understands integer type
promotion better than me can explain why the code is OK (patch->score is
declared as 'int'):

builtin/apply.c:1044:47: warning: comparison of constant 18446744073709551615
    with expression of type 'int' is always false
    [-Wtautological-constant-out-of-range-compare]
        if ((patch->score = strtoul(line, NULL, 10)) == ULONG_MAX)
            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~


> On Wed, Jan 16, 2013 at 11:47 PM, Antoine Pelisse <apelisse@gmail.com> wrote:
> > Create a GREP_HEADER_FIELD_MIN so we can check that the field value is
> > sane and silent the clang warning.
> >
> > Clang warning happens because the enum is unsigned (this is
> > implementation-defined, and there is no negative fields) and the check
> > is then tautological.
> >
> > Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
> > ---
> > I tried to consider discussion [1] and this [2] discussion on clang's list
> >
> > With these two patches and the patch from Max Horne, I'm finally able to
> > compile with CC=clang CFLAGS=-Werror.
> >
> >  [1]: http://thread.gmane.org/gmane.comp.version-control.git/184908
> >  [2]: http://clang-developers.42468.n3.nabble.com/Possibly-invalid-enum-tautology-warning-td3233140.html
> >
> >  grep.c | 3 ++-
> >  grep.h | 3 ++-
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/grep.c b/grep.c
> > index 4bd1b8b..bb548ca 100644
> > --- a/grep.c
> > +++ b/grep.c
> > @@ -625,7 +625,8 @@ static struct grep_expr *prep_header_patterns(struct grep_opt *opt)
> >         for (p = opt->header_list; p; p = p->next) {
> >                 if (p->token != GREP_PATTERN_HEAD)
> >                         die("bug: a non-header pattern in grep header list.");
> > -               if (p->field < 0 || GREP_HEADER_FIELD_MAX <= p->field)
> > +               if (p->field < GREP_HEADER_FIELD_MIN ||
> > +                   GREP_HEADER_FIELD_MAX <= p->field)
> >                         die("bug: unknown header field %d", p->field);
> >                 compile_regexp(p, opt);
> >         }
> > diff --git a/grep.h b/grep.h
> > index 8fc854f..e4a1df5 100644
> > --- a/grep.h
> > +++ b/grep.h
> > @@ -28,7 +28,8 @@ enum grep_context {
> >  };
> >
> >  enum grep_header_field {
> > -       GREP_HEADER_AUTHOR = 0,
> > +       GREP_HEADER_FIELD_MIN = 0,
> > +       GREP_HEADER_AUTHOR = GREP_HEADER_FIELD_MIN,
> >         GREP_HEADER_COMMITTER,
> >         GREP_HEADER_REFLOG,
> >
> > --
> > 1.8.1.1.435.g20d29be.dirty
> >

[PATCH] combine-diff: suppress a clang warning

[John Keeping]

When compiling combine-diff.c, clang 3.2 says:

    combine-diff.c:1006:19: warning: adding 'int' to a string does not
	    append to the string [-Wstring-plus-int]
		prefix = COLONS + offset;
			 ~~~~~~~^~~~~~~~
    combine-diff.c:1006:19: note: use array indexing to silence this warning
		prefix = COLONS + offset;
				^
			 &      [       ]

Suppress this by making the suggested change.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
On Thu, Jan 17, 2013 at 11:00:08AM +0000, John Keeping wrote:
> On Thu, Jan 17, 2013 at 11:32:39AM +0100, Antoine Pelisse wrote:
> There is one remaining warning on pu which hasn't been discussed in this
> thread as far as I can see.  I'll send a patch shortly.

... and here it is.

 combine-diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/combine-diff.c b/combine-diff.c
index bb1cc96..dba4748 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1003,7 +1003,7 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re
 		offset = strlen(COLONS) - num_parent;
 		if (offset < 0)
 			offset = 0;
-		prefix = COLONS + offset;
+		prefix = &COLONS[offset];
 
 		/* Show the modes */
 		for (i = 0; i < num_parent; i++) {
-- 
1.8.1

Re: [PATCH 2/2] fix clang -Wtautological-compare with unsigned enum

[Linus Torvalds]

On Thu, Jan 17, 2013 at 3:00 AM, John Keeping <john@keeping.me.uk> wrote:
>
> There's also a warning that triggers with clang 3.2 but not clang trunk, which
> I think is a legitimate warning - perhaps someone who understands integer type
> promotion better than me can explain why the code is OK (patch->score is
> declared as 'int'):
>
> builtin/apply.c:1044:47: warning: comparison of constant 18446744073709551615
>     with expression of type 'int' is always false
>     [-Wtautological-constant-out-of-range-compare]
>         if ((patch->score = strtoul(line, NULL, 10)) == ULONG_MAX)
>             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~

The warning seems to be very very wrong, and implies that clang has
some nasty bug in it.

Since patch->score is 'int', and UNLONG_MAX is 'unsigned long', the
conversion rules for the comparison is that the int result from the
assignment is cast to unsigned long. And if you cast (int)-1 to
unsigned long, you *do* get ULONG_MAX. That's true regardless of
whether "long" has the same number of bits as "int" or is bigger. The
implicit cast will be done as a sign-extension (unsigned long is not
signed, but the source type of 'int' *is* signed, and that is what
determines the sign extension on casting).

So the "is always false" is pure and utter crap. clang is wrong, and
it is wrong in a way that implies that it actually generates incorrect
code. It may well be worth making a clang bug report about this.

That said, clang is certainly understandably confused. The code
depends on subtle conversion rules and bit patterns, and is clearly
very confusingly written.

So it would probably be good to rewrite it as

    unsigned long val = strtoul(line, NULL, 10);
    if (val == ULONG_MAX) ..
    patch->score = val;

instead. At which point you might as well make the comparison be ">=
INT_MAX" instead, since anything bigger than that is going to be
bogus.

So the git code is probably worth cleaning up, but for git it would be
a cleanup. For clang, this implies a major bug and bad code
generation.

                   Linus
                     Linus

Re: [PATCH 2/2] fix clang -Wtautological-compare with unsigned enum

[Antoine Pelisse]

BTW, I think it has been addressed [1] by clang already and that would
explain why you don't have the warning when using clang trunk version.

[1]: http://llvm-reviews.chandlerc.com/D113

Antoine,

On Thu, Jan 17, 2013 at 5:44 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, Jan 17, 2013 at 3:00 AM, John Keeping <john@keeping.me.uk> wrote:
>>
>> There's also a warning that triggers with clang 3.2 but not clang trunk, which
>> I think is a legitimate warning - perhaps someone who understands integer type
>> promotion better than me can explain why the code is OK (patch->score is
>> declared as 'int'):
>>
>> builtin/apply.c:1044:47: warning: comparison of constant 18446744073709551615
>>     with expression of type 'int' is always false
>>     [-Wtautological-constant-out-of-range-compare]
>>         if ((patch->score = strtoul(line, NULL, 10)) == ULONG_MAX)
>>             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~
>
> The warning seems to be very very wrong, and implies that clang has
> some nasty bug in it.
>
> Since patch->score is 'int', and UNLONG_MAX is 'unsigned long', the
> conversion rules for the comparison is that the int result from the
> assignment is cast to unsigned long. And if you cast (int)-1 to
> unsigned long, you *do* get ULONG_MAX. That's true regardless of
> whether "long" has the same number of bits as "int" or is bigger. The
> implicit cast will be done as a sign-extension (unsigned long is not
> signed, but the source type of 'int' *is* signed, and that is what
> determines the sign extension on casting).
>
> So the "is always false" is pure and utter crap. clang is wrong, and
> it is wrong in a way that implies that it actually generates incorrect
> code. It may well be worth making a clang bug report about this.
>
> That said, clang is certainly understandably confused. The code
> depends on subtle conversion rules and bit patterns, and is clearly
> very confusingly written.
>
> So it would probably be good to rewrite it as
>
>     unsigned long val = strtoul(line, NULL, 10);
>     if (val == ULONG_MAX) ..
>     patch->score = val;
>
> instead. At which point you might as well make the comparison be ">=
> INT_MAX" instead, since anything bigger than that is going to be
> bogus.
>
> So the git code is probably worth cleaning up, but for git it would be
> a cleanup. For clang, this implies a major bug and bad code
> generation.
>
>                    Linus
>                      Linus

Re: [PATCH 2/2] fix clang -Wtautological-compare with unsigned enum

[John Keeping]

On Thu, Jan 17, 2013 at 08:44:20AM -0800, Linus Torvalds wrote:
> On Thu, Jan 17, 2013 at 3:00 AM, John Keeping <john@keeping.me.uk> wrote:
>>
>> There's also a warning that triggers with clang 3.2 but not clang trunk, which
>> I think is a legitimate warning - perhaps someone who understands integer type
>> promotion better than me can explain why the code is OK (patch->score is
>> declared as 'int'):
>>
>> builtin/apply.c:1044:47: warning: comparison of constant 18446744073709551615
>>     with expression of type 'int' is always false
>>     [-Wtautological-constant-out-of-range-compare]
>>         if ((patch->score = strtoul(line, NULL, 10)) == ULONG_MAX)
>>             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~
> 
> The warning seems to be very very wrong, and implies that clang has
> some nasty bug in it.
> 
> Since patch->score is 'int', and UNLONG_MAX is 'unsigned long', the
> conversion rules for the comparison is that the int result from the
> assignment is cast to unsigned long. And if you cast (int)-1 to
> unsigned long, you *do* get ULONG_MAX. That's true regardless of
> whether "long" has the same number of bits as "int" or is bigger. The
> implicit cast will be done as a sign-extension (unsigned long is not
> signed, but the source type of 'int' *is* signed, and that is what
> determines the sign extension on casting).
> 
> So the "is always false" is pure and utter crap. clang is wrong, and
> it is wrong in a way that implies that it actually generates incorrect
> code. It may well be worth making a clang bug report about this.

The warning doesn't occur with a build from their trunk so it looks like
it's already fixed - it just won't make into into a release for about 5
months going by their timeline.

Thanks for the clear explanation.

Re: [PATCH 2/2] fix clang -Wtautological-compare with unsigned enum

[Phil Hord]

On Thu, Jan 17, 2013 at 11:44 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, Jan 17, 2013 at 3:00 AM, John Keeping <john@keeping.me.uk> wrote:
>>
>> There's also a warning that triggers with clang 3.2 but not clang trunk, which
>> I think is a legitimate warning - perhaps someone who understands integer type
>> promotion better than me can explain why the code is OK (patch->score is
>> declared as 'int'):
>>
>> builtin/apply.c:1044:47: warning: comparison of constant 18446744073709551615
>>     with expression of type 'int' is always false
>>     [-Wtautological-constant-out-of-range-compare]
>>         if ((patch->score = strtoul(line, NULL, 10)) == ULONG_MAX)
>>             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~
>
> The warning seems to be very very wrong, and implies that clang has
> some nasty bug in it.
>
> Since patch->score is 'int', and UNLONG_MAX is 'unsigned long', the
> conversion rules for the comparison is that the int result from the
> assignment is cast to unsigned long. And if you cast (int)-1 to
> unsigned long, you *do* get ULONG_MAX. That's true regardless of
> whether "long" has the same number of bits as "int" or is bigger. The
> implicit cast will be done as a sign-extension (unsigned long is not
> signed, but the source type of 'int' *is* signed, and that is what
> determines the sign extension on casting).
>
> So the "is always false" is pure and utter crap. clang is wrong, and
> it is wrong in a way that implies that it actually generates incorrect
> code. It may well be worth making a clang bug report about this.
>
> That said, clang is certainly understandably confused. The code
> depends on subtle conversion rules and bit patterns, and is clearly
> very confusingly written.
>
> So it would probably be good to rewrite it as
>
>     unsigned long val = strtoul(line, NULL, 10);
>     if (val == ULONG_MAX) ..
>     patch->score = val;
>
> instead. At which point you might as well make the comparison be ">=
> INT_MAX" instead, since anything bigger than that is going to be
> bogus.
>
> So the git code is probably worth cleaning up, but for git it would be
> a cleanup. For clang, this implies a major bug and bad code
> generation.


Yes, I can tell by the wording of the error message that you are right
and clang has a problem.  But the git code it complained about does
have a real problem, because the result of "signed int a = ULONG_MAX"
is implementation-defined.  It cannot be guaranteed or expected that
patch->score will ever be assigned -1 there, and so the comparison may
always be false.  I guess the warning is correct, but only
accidentally.  :-)

Your rewrite is more sane and corrects the problem, I think.

Phil

Re: [PATCH 2/2] fix clang -Wtautological-compare with unsigned enum

[Linus Torvalds]

On Fri, Jan 18, 2013 at 9:15 AM, Phil Hord <phil.hord@gmail.com> wrote:
>
> Yes, I can tell by the wording of the error message that you are right
> and clang has a problem.  But the git code it complained about does
> have a real problem, because the result of "signed int a = ULONG_MAX"
> is implementation-defined.

Only theoretically.

Git won't work on machines that don't have 8-bit bytes anyway, so
worrying about the theoretical crazy architectures that aren't two's
complement etc isn't something I'd care about.

There's a whole class of "technically implementation-defined" issues
in C that simply aren't worth caring for. Yes, the standard is written
so that it works on machines that aren't byte-addressable, or EBCDIC
or have things like 18-bit words and 36-bit longwords. Or 16-bit "int"
for microcontrollers etc.

That doesn't make those "implementation-defined" issues worth worrying
about these days. A compiler writer could in theory make up some
idiotic rules that are still "valid by the C standard" even on modern
machines, but such a compiler should simply not be used, and the
compiler writer in question should be called out for being an ass-hat.

Paper standards are only worth so much. And that "so much" really
isn't very much.

                Linus

Re: [PATCH] fix some clang warnings

[Miles Bader]

Jeff King <peff@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.