Re: [PATCH 2/2] fix clang -Wtautological-compare with unsigned enum
On Fri, Jan 18, 2013 at 9:15 AM, Phil Hord 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 -- 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 2/2] fix clang -Wtautological-compare with unsigned enum
On Thu, Jan 17, 2013 at 11:44 AM, Linus Torvalds wrote: > On Thu, Jan 17, 2013 at 3:00 AM, John Keeping 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 -- 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 2/2] fix clang -Wtautological-compare with unsigned enum
On Thu, Jan 17, 2013 at 08:44:20AM -0800, Linus Torvalds wrote: > On Thu, Jan 17, 2013 at 3:00 AM, John Keeping 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. -- 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 2/2] fix clang -Wtautological-compare with unsigned enum
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 wrote: > On Thu, Jan 17, 2013 at 3:00 AM, John Keeping 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 -- 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 2/2] fix clang -Wtautological-compare with unsigned enum
On Thu, Jan 17, 2013 at 3:00 AM, John Keeping 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 -- 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 2/2] fix clang -Wtautological-compare with unsigned enum
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 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
> > ---
> > 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
> >
--
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 2/2] fix clang -Wtautological-compare with unsigned enum
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 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
> ---
> 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
>
--
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 2/2] fix clang -Wtautological-compare with unsigned enum
> With these two patches and the patch from Max Horne, I'm deeply sorry for this typo 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
[PATCH 2/2] fix clang -Wtautological-compare with unsigned enum
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
---
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
--
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
