[Bug c/66773] sign-compare warning for == and != are pretty useless
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66773
Eric Gallager changed:
What|Removed |Added
CC||egallager at gcc dot gnu.org
--- Comment #25 from Eric Gallager ---
(In reply to Segher Boessenkool from comment #16)
> (In reply to Eric Gallager from comment #13)
> > > Yes. You should not use casts, except in some very specific cases, and
> > > most of the uses you see "in the wild" are a bad idea. Sometimes I
> > > wonder if we should have a -Wcast ("Warn for any cast.").
> >
> > I get the feeling that such a warning would be extremely noisy and that
> > no one would use it.
>
> It was not meant as a serious suggestion of course, or I would have done
> this many many years ago.
OK, but I still think some of the ideas I came up with in response to it are
good ideas, though; I'd like to amend this comment #13 in response:
> It would probably be better to go about improving existing cast-related
> warnings, or adding new ones for specific cases, rather than breaking out
> such a broad hammer. For example, the fixits that David Malcolm added for
> -Wold-style-cast are very nice; extending those to apply to more
> cast-related warnings would be a good improvement (I've been meaning to
> open a separate bug about this). These would all be better, more-specific
> warnings to add:
> * -Wcast-to-the-same-type (bug 85043)
> * -Wcast-variadic-function-type (bug 87379)
> * -Wfunctional-cast (bug 69818)
> * -Wcast-enum (bug 30357)
> * -Wold-style-cast-qual (fixit would suggest using const_cast instead)
> * -Wold-style-useless-cast
> * Any of clang's cast-related warnings that we currently don't have yet;
> grep https://clang.llvm.org/docs/DiagnosticsReference.html for the word
> "cast" to find some
To this list, I'd like to add:
* -Wold-style-cast-align (similar to -Wold-style-cast-qual)
* -Wuseless-old-style-cast (as an alternate spelling of
-Wold-style-useless-cast)
[Bug c/66773] sign-compare warning for == and != are pretty useless
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66773 --- Comment #24 from Segher Boessenkool --- (In reply to Daniel Marjamäki from comment #23) > > If the user expects C to provide tests for "mathematically different", the > user has some learning to do. > > I believe most users can appreciate this. But few users fully understand the > integer conversions. I think it's dangerous. :-( But deleting or disabling the sign compare warnings will not help this. > > If, as I said, the user uses explicit casts, that's not good. Much better > already is to use implicit casts, as I said; and much better than that is > to fix the problems at the root (use proper types everywhere), as I said. > > You are de-facto advocating explicit casts because that is how everybody > fixes these. No, I am not; and no, not everyone uses casts to shut up these warnings. Too many people do, certainly, but not even the majority. > The advice to use proper types is good, in my opinion everybody tries to do > that. However as far as I see it's impossible in practice to never mix > signed and unsigned types in real projects. > > > There is no simple fix, so GCC cannot give good guidance. > > Then people just blindly use explicit casts.. and I dislike that. Sure, me too. Do you see a way we can discourage people from doing that?
[Bug c/66773] sign-compare warning for == and != are pretty useless
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66773 --- Comment #23 from Daniel Marjamäki --- > If the user expects C to provide tests for "mathematically different", the user has some learning to do. I believe most users can appreciate this. But few users fully understand the integer conversions. I think it's dangerous. :-( > If, as I said, the user uses explicit casts, that's not good. Much better already is to use implicit casts, as I said; and much better than that is to fix the problems at the root (use proper types everywhere), as I said. You are de-facto advocating explicit casts because that is how everybody fixes these. The advice to use proper types is good, in my opinion everybody tries to do that. However as far as I see it's impossible in practice to never mix signed and unsigned types in real projects. > There is no simple fix, so GCC cannot give good guidance. Then people just blindly use explicit casts.. and I dislike that. > Oh, and don't try to insult me please, I'm much too dumb for that. I apologize! I do not want to insult people.
[Bug c/66773] sign-compare warning for == and != are pretty useless
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66773 --- Comment #22 from Vincent Lefèvre --- (In reply to Segher Boessenkool from comment #21) > If, as I said, the user uses explicit casts, that's not good. Much better > already is to use implicit casts, as I said; There's no such thing as implicit casts. Casts are always explicit. > and much better than that is > to fix the problems at the root (use proper types everywhere), as I said. This will not necessarily solve the problem, because the size and/or the signedness of a type may be unknown (the signedness can be detected with a macro, but there's no way to change the sign of a type).
[Bug c/66773] sign-compare warning for == and != are pretty useless
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66773 --- Comment #21 from Segher Boessenkool --- (In reply to Daniel Marjamäki from comment #20) > Ping. Your "much better" code does not work. I said that this is much better than an explicit cast. It is. And it behaves identically. If the user expects C to provide tests for "mathematically different", the user has some learning to do. If, as I said, the user uses explicit casts, that's not good. Much better already is to use implicit casts, as I said; and much better than that is to fix the problems at the root (use proper types everywhere), as I said. There is no simple fix, so GCC cannot give good guidance. Oh, and don't try to insult me please, I'm much too dumb for that.
[Bug c/66773] sign-compare warning for == and != are pretty useless
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66773
--- Comment #20 from Daniel Marjamäki ---
(In reply to Segher Boessenkool from comment #15)
> (In reply to Daniel Marjamäki from comment #12)
> > So, how would you fix the warning for `f`? Many programmers would "fix" it
> > with a cast.
> >
> > Assuming that `s` and `u` can have arbitrary values, here is the proper
> > code:
> >
> > void f(long s, unsigned long u) { if (s >= 0 && s == u) g(); }
> >
> > For this correct code, gcc warns.
>
> A much better fix is
>
> void f1(long s, unsigned long u) { unsigned long su = s; if (su == u) g(); }
>
> which makes it rather explicit what is going on.
>
> Still much better is to not mixed signedness in types at all.
Ping. Your "much better" code does not work. This code prints "equal" on the
screen:
void f1(long s, unsigned long u) {
unsigned long su = s;
if (su == u) printf("equal\n");
}
int main() { f1(-1L, ~0UL); return 0; }
Please try again.
You proved my point somewhat. The programmer gets a warning, the programmer
tries to fix it, the code still has the same bug but the warning has gone away.
However I feel that your fix is much safer than a cast because Cppcheck,
sanitizers, etc can still warn.
[Bug c/66773] sign-compare warning for == and != are pretty useless
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66773
--- Comment #19 from Daniel Marjamäki ---
(In reply to Segher Boessenkool from comment #15)
> (In reply to Daniel Marjamäki from comment #12)
> > So, how would you fix the warning for `f`? Many programmers would "fix" it
> > with a cast.
> >
> > Assuming that `s` and `u` can have arbitrary values, here is the proper
> > code:
> >
> > void f(long s, unsigned long u) { if (s >= 0 && s == u) g(); }
> >
> > For this correct code, gcc warns.
>
> A much better fix is
>
> void f1(long s, unsigned long u) { unsigned long su = s; if (su == u) g(); }
>
> which makes it rather explicit what is going on.
>
> Still much better is to not mixed signedness in types at all.
That does not work.
Imagine that you call f1 like so : `f1(-1L, ~0UL)`. If you don't want that g()
is called when the argument values are mathematically different.. like here..
how do you do?
[Bug c/66773] sign-compare warning for == and != are pretty useless
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66773
--- Comment #18 from Segher Boessenkool ---
(In reply to Vincent Lefèvre from comment #17)
> (In reply to Segher Boessenkool from comment #15)
> > A much better fix is
> >
> > void f1(long s, unsigned long u) { unsigned long su = s; if (su == u) g(); }
>
> But what if s is some arbitrary integer type, e.g. that comes from a library?
You need to fix this per case. If there was a simple fix this PR would
not even exist :-(
> > Still much better is to not mixed signedness in types at all.
>
> One does not necessarily have the choice. The signedness of some types is
> not specified.
Do not mix signed, unsigned, and unspecified signedness type.
This requires less sloppy API design, and you need to match impedance
with other libraries in some places, sure. Same as always.
[Bug c/66773] sign-compare warning for == and != are pretty useless
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66773
--- Comment #17 from Vincent Lefèvre ---
(In reply to Segher Boessenkool from comment #15)
> A much better fix is
>
> void f1(long s, unsigned long u) { unsigned long su = s; if (su == u) g(); }
But what if s is some arbitrary integer type, e.g. that comes from a library?
Even using uintmax_t may be insufficient. For instance, GCC has __uint128.
> Still much better is to not mixed signedness in types at all.
One does not necessarily have the choice. The signedness of some types is not
specified.
[Bug c/66773] sign-compare warning for == and != are pretty useless
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66773
--- Comment #16 from Segher Boessenkool ---
(In reply to Eric Gallager from comment #13)
> > Yes. You should not use casts, except in some very specific cases, and
> > most of the uses you see "in the wild" are a bad idea. Sometimes I wonder
> > if we should have a -Wcast ("Warn for any cast.").
>
> I get the feeling that such a warning would be extremely noisy and that no
> one would use it.
It was not meant as a serious suggestion of course, or I would have done this
many many years ago.
My point is that *most* casts you see in the wild are a bad idea.
Unfortunately it isn't easy (if possible at all) to have the compiler give
the user good advice on this.
[Bug c/66773] sign-compare warning for == and != are pretty useless
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66773
--- Comment #15 from Segher Boessenkool ---
(In reply to Daniel Marjamäki from comment #12)
> So, how would you fix the warning for `f`? Many programmers would "fix" it
> with a cast.
>
> Assuming that `s` and `u` can have arbitrary values, here is the proper code:
>
> void f(long s, unsigned long u) { if (s >= 0 && s == u) g(); }
>
> For this correct code, gcc warns.
A much better fix is
void f1(long s, unsigned long u) { unsigned long su = s; if (su == u) g(); }
which makes it rather explicit what is going on.
Still much better is to not mixed signedness in types at all.
[Bug c/66773] sign-compare warning for == and != are pretty useless
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66773 --- Comment #14 from Vincent Lefèvre --- (In reply to Segher Boessenkool from comment #11) > Do you have examples of perfectly fine code where you get a warning? In addition to Daniel's example, I would say that one can have types that are signed but the values are always nonnegative in practice (the goal of having signed types is to be able to do signed arithmetic on them when using other signed types). For instance, this is the case of mpfr_prec_t in GNU MPFR (this contains a precision). Thus it is fine to compare it with a value of type unsigned. And adding casts to avoid the warning (in addition to being a hack) is not even always possible, because one may not know the associated unsigned type.
[Bug c/66773] sign-compare warning for == and != are pretty useless
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66773
--- Comment #13 from Eric Gallager ---
(In reply to Segher Boessenkool from comment #11)
> Hi Daniel,
>
> (In reply to Daniel Marjamäki from comment #9)
> > Problems;
> >
> > * Code that performs comparison properly gets a warning.
>
> You get a warning if you compare a signed thing to an unsigned thing, and
> that signed thing is not (necessarily) the same when cast to unsigned (which
> is what will happen, and is what often catches people by surprise, and that
> is exactly what the warning is for).
>
> Do you have examples of perfectly fine code where you get a warning?
>
> > * Code where programmer makes a mistake with a cast does not generate a
> > warning.
>
> Yes. You should not use casts, except in some very specific cases, and
> most of the uses you see "in the wild" are a bad idea. Sometimes I wonder
> if we should have a -Wcast ("Warn for any cast.").
I get the feeling that such a warning would be extremely noisy and that no one
would use it. It would probably be better to go about improving existing
cast-related warnings, or adding new ones for specific cases, rather than
breaking out such a broad hammer. For example, the fixits that David Malcolm
added for -Wold-style-cast are very nice; extending those to apply to more
cast-related warnings would be a good improvement (I've been meaning to open a
separate bug about this). These would all be better, more-specific warnings to
add than an extremely broad -Wcast:
* -Wcast-to-the-same-type (bug 85043)
* -Wcast-variadic-function-type (bug 87379)
* -Wfunctional-cast (bug 69818)
* -Wcast-enum (bug 30357)
* -Wold-style-cast-qual (fixit would suggest using const_cast instead)
* -Wold-style-useless-cast
* Any of clang's cast-related warnings that we currently don't have yet; grep
https://clang.llvm.org/docs/DiagnosticsReference.html for the word "cast" to
find some
>
> > * This warning encourage programmers to cast and when they do make mistakes
> > sometimes there is no warning.
>
> What?
>
> ===
> void g(void);
> void f(long s, unsigned long u) { if (s == u) g(); }
> void f4(unsigned long u) { if (u == 4) g(); }
> void f8(signed char s, unsigned char u) { if (s == u) g(); }
> ===
>
> su.c: In function 'f':
> su.c:2:41: warning: comparison of integer expressions of different
> signedness: 'long int' and 'long unsigned int' [-Wsign-compare]
> 2 | void f(long s, unsigned long u) { if (s == u) g(); }
> | ^~
>
> Where does that encourage programmers to do silly and dangerous things?
> (Also not f4 and f8 don't warn).
>
> > Yet you think this is all fine and you are happy about these problems.
>
> I think that the warning does what it is supposed to do, yes.
>
> > It's safer to shut off this warning and use better tools to find these
> > issues.
>
> It is even safer to use a different programming language that does not
> have these problems. But we don't, we are programming in C here (or C++
> or whatever), and it has these problems, and we have a warning to warn
> about it.
>
> How can we educate people how to write better code, in the warning message,
> or maybe a fixit hint, or some info message?
If gcc can come up with a fixit, yes, that would be good.
(In reply to Daniel Marjamäki from comment #10)
> Well I am just a happy gcc user.. if some gcc maintainer thinks this ticket
> is invalid feel free to close it. I can't expect that everybody will think
> just like me. :-)
>
> As a Cppcheck developer I am dissappointed that all compilers have these
> warnings. And it ruins our chances to find real bugs with proper analysis.
Should we have this block the cppcheck meta-bug (bug 89863) then?
[Bug c/66773] sign-compare warning for == and != are pretty useless
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66773
--- Comment #12 from Daniel Marjamäki ---
> Do you have examples of perfectly fine code where you get a warning?
So, how would you fix the warning for `f`? Many programmers would "fix" it with
a cast.
Assuming that `s` and `u` can have arbitrary values, here is the proper code:
void f(long s, unsigned long u) { if (s >= 0 && s == u) g(); }
For this correct code, gcc warns.
[Bug c/66773] sign-compare warning for == and != are pretty useless
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66773
--- Comment #11 from Segher Boessenkool ---
Hi Daniel,
(In reply to Daniel Marjamäki from comment #9)
> Problems;
>
> * Code that performs comparison properly gets a warning.
You get a warning if you compare a signed thing to an unsigned thing, and
that signed thing is not (necessarily) the same when cast to unsigned (which
is what will happen, and is what often catches people by surprise, and that
is exactly what the warning is for).
Do you have examples of perfectly fine code where you get a warning?
> * Code where programmer makes a mistake with a cast does not generate a
> warning.
Yes. You should not use casts, except in some very specific cases, and
most of the uses you see "in the wild" are a bad idea. Sometimes I wonder
if we should have a -Wcast ("Warn for any cast.").
> * This warning encourage programmers to cast and when they do make mistakes
> sometimes there is no warning.
What?
===
void g(void);
void f(long s, unsigned long u) { if (s == u) g(); }
void f4(unsigned long u) { if (u == 4) g(); }
void f8(signed char s, unsigned char u) { if (s == u) g(); }
===
su.c: In function 'f':
su.c:2:41: warning: comparison of integer expressions of different signedness:
'long int' and 'long unsigned int' [-Wsign-compare]
2 | void f(long s, unsigned long u) { if (s == u) g(); }
| ^~
Where does that encourage programmers to do silly and dangerous things?
(Also not f4 and f8 don't warn).
> Yet you think this is all fine and you are happy about these problems.
I think that the warning does what it is supposed to do, yes.
> It's safer to shut off this warning and use better tools to find these
> issues.
It is even safer to use a different programming language that does not
have these problems. But we don't, we are programming in C here (or C++
or whatever), and it has these problems, and we have a warning to warn
about it.
How can we educate people how to write better code, in the warning message,
or maybe a fixit hint, or some info message?
[Bug c/66773] sign-compare warning for == and != are pretty useless
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66773 --- Comment #10 from Daniel Marjamäki --- Well I am just a happy gcc user.. if some gcc maintainer thinks this ticket is invalid feel free to close it. I can't expect that everybody will think just like me. :-) As a Cppcheck developer I am dissappointed that all compilers have these warnings. And it ruins our chances to find real bugs with proper analysis.
[Bug c/66773] sign-compare warning for == and != are pretty useless
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66773 --- Comment #9 from Daniel Marjamäki --- Problems; * Code that performs comparison properly gets a warning. * Code where programmer makes a mistake with a cast does not generate a warning. * This warning encourage programmers to cast and when they do make mistakes sometimes there is no warning. Yet you think this is all fine and you are happy about these problems. It's safer to shut off this warning and use better tools to find these issues. The casts will silence i.e. Cppcheck so it's safer to avoid the casts (it can't know if loss of data is by intention or by mistake when you tried to perform a sign-cast).
[Bug c/66773] sign-compare warning for == and != are pretty useless
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66773
--- Comment #8 from Segher Boessenkool ---
int f0(void) { return 0x == -1; }
int f1(unsigned x) { return x == -1; }
int f2(int y) { return 0x == y; }
int f3(unsigned x, int y) { return x == y; }
All of them warn the same, and that is good, imo.
[Bug c/66773] sign-compare warning for == and != are pretty useless
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66773
Vincent Lefèvre changed:
What|Removed |Added
CC||vincent-gcc at vinc17 dot net
--- Comment #7 from Vincent Lefèvre ---
(In reply to Segher Boessenkool from comment #1)
> There certainly are cases where these warnings are inconvenient, but
> there also are cases where they are quite useful -- e.g.
>
> int f(void) { return 0x == -1; }
I'd say that the warning is inconvenient here, since if the developer really
wants to do a test equivalent to the above one, there seems no good way to
avoid the warning with portable code.
[Bug c/66773] sign-compare warning for == and != are pretty useless
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66773 Eric Gallager changed: What|Removed |Added CC||egall at gwmail dot gwu.edu --- Comment #6 from Eric Gallager --- (In reply to Andreas Schwab from comment #5) > A cast is seldom a good solution Well, that's the sort of solution this warning causes inexperienced programmers who don't know any better (such as myself) to use... Maybe now that GCC has fixits, it could suggest something better?
[Bug c/66773] sign-compare warning for == and != are pretty useless
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66773 --- Comment #5 from Andreas Schwab sch...@linux-m68k.org --- A cast is seldom a good solution, but even equality tests have the potential to go wrong with C's composite type rules.
[Bug c/66773] sign-compare warning for == and != are pretty useless
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66773
Segher Boessenkool segher at gcc dot gnu.org changed:
What|Removed |Added
CC||segher at gcc dot gnu.org
--- Comment #1 from Segher Boessenkool segher at gcc dot gnu.org ---
There certainly are cases where these warnings are inconvenient, but
there also are cases where they are quite useful -- e.g.
int f(void) { return 0x == -1; }
[Bug c/66773] sign-compare warning for == and != are pretty useless
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66773 --- Comment #3 from Andreas Schwab sch...@linux-m68k.org --- It's always the boundary cases that matter most for security.
[Bug c/66773] sign-compare warning for == and != are pretty useless
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66773
--- Comment #2 from Daniel Marjamäki daniel.marjamaki at gmail dot com ---
Thanks!
Hmm.. in my humble opinion, when I see the code:
int f(void) { return 0x == -1; }
.. I get the impression that the developer probably wants to test if the
bitpattern 0xfff.. matches -1.
I'd say an arbitrary U32 variable will rarelly have such large values unless
it's representing bitpatterns.. indexes, positions, sizes, etc are not that
large. and if you match a bitpattern against a negative value I'd say the match
is probably expected.
Is it possible to test how much noise this generates? My feeling is that if I
run this on various open source projects I will get lots of pure noise. If I am
right, do you think such noise would be convincing?
[Bug c/66773] sign-compare warning for == and != are pretty useless
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66773 --- Comment #4 from Daniel Marjamäki daniel.marjamaki at gmail dot com --- absolutely. there are often bugs in the boundaries. well. I was hoping to get more optimistic response. how about this.. imagine that we wrote a possible division by zero warning for every integer division that uses a non-constant rhs. every warning where rhs can't really be zero would be a false positive. That would be very noisy imo. imho the false positive rate should be similar here. If there is a comparison 'a == -1' and a is unsigned then this warning is useless if a can't be 0x. if a has arbitrary values then statistically it's as likely that a is 0x and 0. So I guess the false positive rate is somewhat similar. Maybe the message can be tweaked? comparison between signed and unsigned integer expressions [-Wsign-compare] I think this message is fine for relational comparisons. A sign-cast is a reasonable solution. For == and != I am afraid the message is somewhat misleading. A sign-cast is not a good solution.
