digitalmars.D.bugs - [Issue 6949] New: no warning or error if unsigned variable is compared to 0
- d-bugmail puremagic.com (26/26) Nov 14 2011 http://d.puremagic.com/issues/show_bug.cgi?id=6949
- d-bugmail puremagic.com (9/9) Nov 16 2011 http://d.puremagic.com/issues/show_bug.cgi?id=6949
- d-bugmail puremagic.com (12/12) Nov 29 2012 http://d.puremagic.com/issues/show_bug.cgi?id=6949
- d-bugmail puremagic.com (26/26) Nov 29 2012 http://d.puremagic.com/issues/show_bug.cgi?id=6949
- d-bugmail puremagic.com (14/16) Nov 29 2012 http://d.puremagic.com/issues/show_bug.cgi?id=6949
- d-bugmail puremagic.com (9/11) Nov 29 2012 http://d.puremagic.com/issues/show_bug.cgi?id=6949
- d-bugmail puremagic.com (7/9) Nov 29 2012 http://d.puremagic.com/issues/show_bug.cgi?id=6949
- d-bugmail puremagic.com (16/16) Nov 29 2012 http://d.puremagic.com/issues/show_bug.cgi?id=6949
- d-bugmail puremagic.com (7/22) Nov 29 2012 http://d.puremagic.com/issues/show_bug.cgi?id=6949
- d-bugmail puremagic.com (20/21) Nov 29 2012 http://d.puremagic.com/issues/show_bug.cgi?id=6949
- d-bugmail puremagic.com (9/9) Nov 29 2012 http://d.puremagic.com/issues/show_bug.cgi?id=6949
- d-bugmail puremagic.com (15/22) Nov 29 2012 http://d.puremagic.com/issues/show_bug.cgi?id=6949
- d-bugmail puremagic.com (11/13) Nov 29 2012 http://d.puremagic.com/issues/show_bug.cgi?id=6949
- d-bugmail puremagic.com (17/17) Nov 30 2012 http://d.puremagic.com/issues/show_bug.cgi?id=6949
- d-bugmail puremagic.com (17/31) Nov 30 2012 http://d.puremagic.com/issues/show_bug.cgi?id=6949
- d-bugmail puremagic.com (8/25) Nov 30 2012 http://d.puremagic.com/issues/show_bug.cgi?id=6949
- d-bugmail puremagic.com (10/31) Nov 30 2012 http://d.puremagic.com/issues/show_bug.cgi?id=6949
- d-bugmail puremagic.com (27/31) Nov 30 2012 http://d.puremagic.com/issues/show_bug.cgi?id=6949
- d-bugmail puremagic.com (6/6) Dec 18 2012 http://d.puremagic.com/issues/show_bug.cgi?id=6949
http://d.puremagic.com/issues/show_bug.cgi?id=6949
Summary: no warning or error if unsigned variable is compared
to 0
Product: D
Version: D1 & D2
Platform: All
OS/Version: All
Status: NEW
Keywords: diagnostic
Severity: normal
Priority: P2
Component: DMD
AssignedTo: nobody puremagic.com
ReportedBy: mrmocool gmx.de
--- Comment #0 from Trass3r <mrmocool gmx.de> 2011-11-14 18:15:49 PST ---
void main()
{
uint i = 0;
if (i < 0)
assert(0, "never");
}
I think such a case means that something went subtly wrong and the variable's
type was intended to be signed, so there should be an error or warning.
--
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 14 2011
d-bugmail puremagic.com
http://d.puremagic.com/issues/show_bug.cgi?id=6949
Walter Bright <bugzilla digitalmars.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |bugzilla digitalmars.com
Severity|normal |enhancement
--
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 16 2011
http://d.puremagic.com/issues/show_bug.cgi?id=6949
Andrej Mitrovic <andrej.mitrovich gmail.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Keywords| |pull
CC| |andrej.mitrovich gmail.com
AssignedTo|nobody puremagic.com |andrej.mitrovich gmail.com
--- Comment #1 from Andrej Mitrovic <andrej.mitrovich gmail.com> 2012-11-29
16:14:56 PST ---
https://github.com/D-Programming-Language/dmd/pull/1337
--
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 29 2012
http://d.puremagic.com/issues/show_bug.cgi?id=6949
bearophile_hugs eml.cc changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |bearophile_hugs eml.cc
--- Comment #2 from bearophile_hugs eml.cc 2012-11-29 16:26:50 PST ---
I don't know if Walter will accept this change, but surely I welcome it a lot.
I have desired this in D since years.
For me one important use case of this warning is during code refactoring. Let's
say I have written code that uses some ints. Later for some reasons I turn one
or more of those ints into uints or size_ts. In such situation a warning like
this becomes very useful for me (I know it's useful because this warning has
avoided me troubles several times in C-GCC) because the warnings put in
evidence where the program contains code like "if (x < 0)". This means the code
requires that variable to be signed, or the program logic needs some changes.
Either way, the warning tells me the change I was doing is wrong and needs to
be undone or needs further changes.
Regarding how much common this kind of bug is in already debugged famous open
source programs, there is a kind of static analysis tool that has shown to spot
tens of similar bugs in that code. So this bug is common enough.
The disadvantages of this warning are probably some spurious warnings in
templated code. Is Phobos giving some warnings with this patch? More study is
probably needed.
--
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 29 2012
http://d.puremagic.com/issues/show_bug.cgi?id=6949 --- Comment #3 from Andrej Mitrovic <andrej.mitrovich gmail.com> 2012-11-29 16:40:59 PST --- (In reply to comment #2)Is Phobos giving some warnings with this patch? More study is probably needed.The autotester is running. It failed once because of this: enum En : uint { a, // 0 b // triggers comparison } I've added a workaround so the warning isn't triggered in this case anymore. We'll see what the tester says.. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Nov 29 2012
http://d.puremagic.com/issues/show_bug.cgi?id=6949 --- Comment #4 from bearophile_hugs eml.cc 2012-11-29 17:20:37 PST --- (In reply to comment #3)I've added a workaround so the warning isn't triggered in this case anymore. We'll see what the tester says..OK. It seems Andrei has appreciated this. In your code this comment is obsolete: // Warn when unsigned type is ... -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Nov 29 2012
http://d.puremagic.com/issues/show_bug.cgi?id=6949 --- Comment #5 from Andrej Mitrovic <andrej.mitrovich gmail.com> 2012-11-29 17:21:48 PST --- (In reply to comment #4)In your code this comment is obsolete: // Warn when unsigned type is ...Thanks. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Nov 29 2012
http://d.puremagic.com/issues/show_bug.cgi?id=6949
--- Comment #6 from bearophile_hugs eml.cc 2012-11-29 18:03:20 PST ---
How about code like this?
void main() {
uint i = 0;
if (i == -2)
assert(0, "never");
}
Note that this is currently valid D code:
void main() {
uint i = -2;
if (i == -2) {}
}
--
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 29 2012
http://d.puremagic.com/issues/show_bug.cgi?id=6949
--- Comment #7 from Andrej Mitrovic <andrej.mitrovich gmail.com> 2012-11-29
18:14:27 PST ---
(In reply to comment #6)
How about code like this?
void main() {
uint i = 0;
if (i == -2)
assert(0, "never");
}
Note that this is currently valid D code:
void main() {
uint i = -2;
if (i == -2) {}
}
I want to see how Walter reacts to the pull before these are handled.
--
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 29 2012
http://d.puremagic.com/issues/show_bug.cgi?id=6949 --- Comment #8 from bearophile_hugs eml.cc 2012-11-29 18:17:45 PST --- (In reply to comment #7)I want to see how Walter reacts to the pull before these are handled.Right, OK. This new error message of yours hits code like this in std.bitmanip: enum result = " property safe "~T.stringof~" "~name~"() pure nothrow const { auto result = " "("~store~" & " ~ myToString(maskAllElse) ~ ") >>" ~ myToString(offset) ~ ";" ~ (T.min < 0 ? "if (result >= " ~ myToString(signBitCheck) ~ ") result |= " ~ myToString(extendSign) ~ ";" : "") ~ " return cast("~T.stringof~") result;}\n" Unless D grows a "static ternary operator" (that is usable in this case) it's not easy to solve such situations. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Nov 29 2012
http://d.puremagic.com/issues/show_bug.cgi?id=6949 --- Comment #9 from bearophile_hugs eml.cc 2012-11-29 18:22:17 PST --- Maybe you want to temporarily turn your new error message into a warning (that later will become an error again, if you want), so if Phobos gets entirely compiled with "-wi" it will not stop the compilation and the log will show the possible bugs spotted by this warning. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Nov 29 2012
http://d.puremagic.com/issues/show_bug.cgi?id=6949
--- Comment #10 from Walter Bright <bugzilla digitalmars.com> 2012-11-29
19:50:14 PST ---
(In reply to comment #6)
How about code like this?
void main() {
uint i = 0;
if (i == -2)
assert(0, "never");
}
Equality has nothing to do with sign, i.e. there is no "signed equality" and
"unsigned equality". There's just equality.
I don't think there's any getting away from the fact that in languages like D
and C, signed and unsigned are simply different ways of viewing the same data.
For example, you can offset a pointer with both signed and unsigned values.
There is no clean separation between the two.
Some languages, such as Java, deal with this duality by defining the unsigned
type out of existence.
I've made more comments in the pull request.
--
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 29 2012
http://d.puremagic.com/issues/show_bug.cgi?id=6949 --- Comment #11 from bearophile_hugs eml.cc 2012-11-29 20:15:15 PST --- (In reply to comment #10)Some languages, such as Java, deal with this duality by defining the unsigned type out of existence.There are languages like Ada, that have both signed and unsigned native integrals, but behave differently from C/C++/D. (Even if this patch is eventually refused, I suggest to turn it into a warning and compile all phobos one time again with "-wi" to look for possible bugs spotted by this.) -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Nov 29 2012
http://d.puremagic.com/issues/show_bug.cgi?id=6949
--- Comment #12 from bearophile_hugs eml.cc 2012-11-30 04:17:15 PST ---
Regarding a comment by Don, is it possible to generate a warning/error only in
foo() and not in bar()?
void foo(uint x) {
if (x < 0) {} // error or warning here
}
void bar(T)(T x) {
if (x < 0) {} // OK
}
void main() {
foo(5U);
bar(5U);
}
--
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 30 2012
http://d.puremagic.com/issues/show_bug.cgi?id=6949
Andrej Mitrovic <andrej.mitrovich gmail.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Keywords|pull |
AssignedTo|andrej.mitrovich gmail.com |nobody puremagic.com
--- Comment #13 from Andrej Mitrovic <andrej.mitrovich gmail.com> 2012-11-30
04:43:25 PST ---
(In reply to comment #12)
Regarding a comment by Don, is it possible to generate a warning/error only in
foo() and not in bar()?
void foo(uint x) {
if (x < 0) {} // error or warning here
}
void bar(T)(T x) {
if (x < 0) {} // OK
}
void main() {
foo(5U);
bar(5U);
}
I doubt it. Anyway I've closed the pull for now since it's controversial when
(or if) we should have warnings/errors for this.
If we had a switch like GCC's `-Wtype-limits` then we could implement this
check for all cases when the switch is enabled and not worry about it. But
Walter is against switches so...
Maybe LDC/GDC can or already do support this though.
--
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 30 2012
http://d.puremagic.com/issues/show_bug.cgi?id=6949 --- Comment #14 from bearophile_hugs eml.cc 2012-11-30 05:25:50 PST --- (In reply to comment #13)(In reply to comment #12)I'd like a better answer about this, maybe from Don or Hara or someone that knows this answer better. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------Regarding a comment by Don, is it possible to generate a warning/error only in foo() and not in bar()? void foo(uint x) { if (x < 0) {} // error or warning here } void bar(T)(T x) { if (x < 0) {} // OK } void main() { foo(5U); bar(5U); }I doubt it.
Nov 30 2012
http://d.puremagic.com/issues/show_bug.cgi?id=6949 --- Comment #15 from Don <clugdbug yahoo.com.au> 2012-11-30 06:31:17 PST --- (In reply to comment #14)(In reply to comment #13)It's easy. DSymbol.inTemplateInstance() returns non-NULL if you are inside a template. You just need to call it on the function you're in. BTW std.bigint has a lot of examples of valid comparisons of unsigned < 0 . -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------(In reply to comment #12)I'd like a better answer about this, maybe from Don or Hara or someone that knows this answer better.Regarding a comment by Don, is it possible to generate a warning/error only in foo() and not in bar()? void foo(uint x) { if (x < 0) {} // error or warning here } void bar(T)(T x) { if (x < 0) {} // OK } void main() { foo(5U); bar(5U); }I doubt it.
Nov 30 2012
http://d.puremagic.com/issues/show_bug.cgi?id=6949 --- Comment #16 from bearophile_hugs eml.cc 2012-11-30 09:36:58 PST --- (In reply to comment #15)BTW std.bigint has a lot of examples of valid comparisons of unsigned < 0 .The comparisons in std.bigint are like this, all of them are inside templated functions: BigInt opAssign(T: long)(T x) { data = cast(ulong)((x < 0) ? -x : x); sign = (x < 0); return this; } BigInt opOpAssign(string op, T)(T y) if ((op=="+" || op=="-" || op=="*" || op=="/" || op=="%" || op==">>" || op=="<<" || op=="^^") && is (T: long)) { ulong u = cast(ulong)(y < 0 ? -y : y); ...It's easy. DSymbol.inTemplateInstance() returns non-NULL if you are inside a template. You just need to call it on the function you're in.Thank you Don. If it's not hard to do then I suggest to add that in this line to exclude templates from this test, and take a look at what the Phobos autotester says: // Error when unsigned type is compared less-than to zero, but not in enum declarations or in static if if ((!(sc->flags & SCOPEstaticif)) && sc->parent && !sc->parent->isEnumDeclaration()) -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Nov 30 2012
http://d.puremagic.com/issues/show_bug.cgi?id=6949 --- Comment #17 from Andrej Mitrovic <andrej.mitrovich gmail.com> 2012-12-18 09:19:32 PST --- *** Issue 5539 has been marked as a duplicate of this issue. *** -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Dec 18 2012