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

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

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

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

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

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

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

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

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

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