www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Why is this not a warning?

reply Shachar Shemesh <shachar weka.io> writes:
Please consider the following program, which is a reduced version of a 
problem I've spent the entire of today trying to debug:

import std.stdio;

void main() {
     enum ulong MAX_VAL = 256;
     long value = -500;

     if( value>MAX_VAL )
         value = MAX_VAL;

     writeln(value);
}

People who are marginally familiar with integer promotion will not be 
surprised to know that the program prints "256". What is surprising to 
me is that this produced neither error nor warning.

The comparable program in C++, when compiled with gcc, correctly warns 
about signed/unsigned comparison (though, to be fair, it seems that 
clang doesn't).
Mar 16 2016
next sibling parent Kagamin <spam here.lot> writes:
https://issues.dlang.org/show_bug.cgi?id=259
Mar 16 2016
prev sibling next sibling parent jmh530 <john.michael.hall gmail.com> writes:
On Wednesday, 16 March 2016 at 16:40:49 UTC, Shachar Shemesh 
wrote:
 People who are marginally familiar with integer promotion will 
 not be surprised to know that the program prints "256". What is 
 surprising to me is that this produced neither error nor 
 warning.
Here's a simpler example: import std.stdio : writeln; void main() { ulong MAX_VAL = 256; long value = -500; writeln(value > MAX_VAL); //prints true } Looks like integer promotion in D follows the same rules as C: when the types are the same size, the signed type is converted to the unsigned type. Definitely could cause an issue and deserves a warning, as you say. It's not like you could switch the rule to being that an unsigned type is converted to the signed type. You would still get errors, just over a different range. You could instead write something like: bool compare(long a, ulong b) { if (b < long.sizeof) return a > cast(long)b; else return cast(float)a > cast(float)b; }
Mar 16 2016
prev sibling next sibling parent reply Mathias Lang <pro.mathias.lang gmail.com> writes:
On Wednesday, 16 March 2016 at 16:40:49 UTC, Shachar Shemesh 
wrote:
 Please consider the following program, which is a reduced 
 version of a problem I've spent the entire of today trying to 
 debug:

 import std.stdio;

 void main() {
     enum ulong MAX_VAL = 256;
     long value = -500;

     if( value>MAX_VAL )
         value = MAX_VAL;

     writeln(value);
 }

 People who are marginally familiar with integer promotion will 
 not be surprised to know that the program prints "256". What is 
 surprising to me is that this produced neither error nor 
 warning.

 The comparable program in C++, when compiled with gcc, 
 correctly warns about signed/unsigned comparison (though, to be 
 fair, it seems that clang doesn't).
I find integer promotion/comparison rules to be one of the messier part of D at the moment. E.g. the compiler won't say anything about `ulong foo = -1;` either. Sadly, to solve that without imposing much pain on the users, you need a more decent VRP than we currently have, for example the following should compile: ``` void main () { ushort f; uint i; if (i < ushort.max) f = i; } ```
Mar 16 2016
next sibling parent jmh530 <john.michael.hall gmail.com> writes:
On Wednesday, 16 March 2016 at 18:39:36 UTC, Mathias Lang wrote:
 Sadly, to solve that without imposing much pain on the users, 
 you need a more decent VRP than we currently have,
I just read Walter's Dr. Dobbs article on VRP: http://www.drdobbs.com/tools/value-range-propagation/229300211
Mar 16 2016
prev sibling parent tsbockman <thomas.bockman gmail.com> writes:
On Wednesday, 16 March 2016 at 18:39:36 UTC, Mathias Lang wrote:
 Sadly, to solve that without imposing much pain on the users, 
 you need a more decent VRP than we currently have...
Lionello Lunesu did a lot of work both on improving VRP, and on bug 259: https://github.com/D-Programming-Language/dmd/pull/1913 He pretty much gave up after being ignored for well over a year. I started porting his work to DDMD back in October.: https://github.com/D-Programming-Language/dmd/pull/5229 I also have an actual fix for issue #259 on my hard drive; I haven't submitted it yet because I need PR 5229 accepted first, and also because there is one other big VRP related improvement that is needed, too. My work is stalled now, just like Lionello's, because: 1) DMD 14835 blocks any meaningful VRP improvements, and 2) No one will do a serious review of my work, either - even though PR 5229 is fairly small and straightforward. Bug 259 could be fixed pretty quickly if it were actually a priority for the core compiler dev team. As it is, it may never get fixed...
Mar 16 2016
prev sibling next sibling parent reply tsbockman <thomas.bockman gmail.com> writes:
On Wednesday, 16 March 2016 at 16:40:49 UTC, Shachar Shemesh 
wrote:
 ...

 People who are marginally familiar with integer promotion will 
 not be surprised to know that the program prints "256". What is 
 surprising to me is that this produced neither error nor 
 warning.

 The comparable program in C++, when compiled with gcc, 
 correctly warns about signed/unsigned comparison (though, to be 
 fair, it seems that clang doesn't).
While this particular issue can and should be addressed in the compiler (see my other reply), in the mean time how about trying out my `checkedint` DUB package? It fixes the signed/unsigned mess, and many other similar problems, such as integer overflow: https://code.dlang.org/packages/checkedint My intention is to submit it for inclusion in Phobos some time soon. Note that because of various flakiness with DMD, it sort of requires LDC or GDC at the moment.
Mar 16 2016
parent reply Shachar Shemesh <shachar weka.io> writes:
On 16/03/16 23:50, tsbockman wrote:
 On Wednesday, 16 March 2016 at 16:40:49 UTC, Shachar Shemesh wrote:
 ...

 People who are marginally familiar with integer promotion will not be
 surprised to know that the program prints "256". What is surprising to
 me is that this produced neither error nor warning.

 The comparable program in C++, when compiled with gcc, correctly warns
 about signed/unsigned comparison (though, to be fair, it seems that
 clang doesn't).
While this particular issue can and should be addressed in the compiler (see my other reply), in the mean time how about trying out my `checkedint` DUB package?
I'm afraid that paying run-time cost to verify correctness is out of the question for the type of product weka is building.
Mar 16 2016
parent reply tsbockman <thomas.bockman gmail.com> writes:
On Thursday, 17 March 2016 at 06:55:34 UTC, Shachar Shemesh wrote:
 On 16/03/16 23:50, tsbockman wrote:
 On Wednesday, 16 March 2016 at 16:40:49 UTC, Shachar Shemesh 
 wrote:
 ...

 People who are marginally familiar with integer promotion 
 will not be
 surprised to know that the program prints "256". What is 
 surprising to
 me is that this produced neither error nor warning.

 The comparable program in C++, when compiled with gcc, 
 correctly warns
 about signed/unsigned comparison (though, to be fair, it 
 seems that
 clang doesn't).
While this particular issue can and should be addressed in the compiler (see my other reply), in the mean time how about trying out my `checkedint` DUB package?
I'm afraid that paying run-time cost to verify correctness is out of the question for the type of product weka is building.
You could use the `DebugInt` wrapper. It aliases to `SafeInt` in debug and unittest mode, to find problems (many, including the specific one in this thread, are detected at compile time). Then, in release mode, it aliases to the built-in types for maximum speed. Also, for almost all real world programs only a small percentage of the code actually affects performance much. Using `SafeInt` or `SmartInt`*everywhere* in a release build will reduce performance by about 30% - but using it *almost* everywhere, except in the program's hot spots, shouldn't measurably reduce performance at all.
Mar 17 2016
next sibling parent reply Dominikus Dittes Scherkl <Dominikus.Scherkl continental-corporation.com> writes:
On Thursday, 17 March 2016 at 09:13:34 UTC, tsbockman wrote:
 On Thursday, 17 March 2016 at 06:55:34 UTC, Shachar Shemesh 
 wrote:
 On 16/03/16 23:50, tsbockman wrote:
 On Wednesday, 16 March 2016 at 16:40:49 UTC, Shachar Shemesh 
 wrote:
 ...

 People who are marginally familiar with integer promotion 
 will not be
 surprised to know that the program prints "256". What is 
 surprising to
 me is that this produced neither error nor warning.

 The comparable program in C++, when compiled with gcc, 
 correctly warns
 about signed/unsigned comparison (though, to be fair, it 
 seems that
 clang doesn't).
While this particular issue can and should be addressed in the compiler (see my other reply), in the mean time how about trying out my `checkedint` DUB package?
I'm afraid that paying run-time cost to verify correctness is out of the question for the type of product weka is building.
You could use the `DebugInt` wrapper. It aliases to `SafeInt` in debug and unittest mode, to find problems (many, including the specific one in this thread, are detected at compile time). Then, in release mode, it aliases to the built-in types for maximum speed. Also, for almost all real world programs only a small percentage of the code actually affects performance much. Using `SafeInt` or `SmartInt`*everywhere* in a release build will reduce performance by about 30% - but using it *almost* everywhere, except in the program's hot spots, shouldn't measurably reduce performance at all.
Or you can use an improved opCmp implementation in the compiler, that only add additional runtime cost, if someone is stupid enough to compare signed with unsigned values - but yield the correct result: int opCmp(T)(const(T) a, const(T) b) pure safe nogc nothrow if(isNumeric!T) { // Should be buildin. Naïve implementation: return (a <= b) ? (a != b) ? -1 : 0 : 1; } /// Returns negative value if a < b, 0 if they are equal or positive value if a > b. /// This will always yield a correct result, no matter which integral types are compared. /// It uses one extra comparison operation if and only if /// one type is signed and the other unsigned but has bigger max. /// For comparison with floating point values the buildin /// operations have no problem, so we don't handle them here. int opCmp(T, U)(const(T) a, const(U) b) pure safe nogc nothrow if(isIntegral!T && isIntegral!U && !is(Unqual!T == Unqual!U)) { alias C = CommonType!(T, U); static if(isSigned!T && isUnsigned!U && T.sizeof <= U.sizeof) return (a < 0) ? -1 : opCmp(cast(U)a, b); else static if(isUnsigned!T && isSigned!U && T.sizeof >= U.sizeof) return (b < 0) ? 1 : opCmp(a, cast(T)b); else return opCmp(cast(C)a, cast(C)b); } unittest { byte a = -2; short c = -2; int e = -2; long g = -2; ubyte b = 4; ushort d = 4; uint f = 4; ulong h = 4; assert(opCmp(a, b) < 0); assert(opCmp(c, d) < 0); assert(opCmp(e, f) < 0); assert(opCmp(g, h) < 0); assert(opCmp(a, h) < 0); assert(opCmp(b, g) > 0); assert(opCmp(d, e) > 0); // compiler test: assert(a < b); assert(c < d); assert(e < f); // fails assert(g < h); // fails assert(a < h); // fails assert(b > g); assert(d > e); }
Mar 17 2016
next sibling parent tsbockman <thomas.bockman gmail.com> writes:
On Thursday, 17 March 2016 at 09:59:41 UTC, Dominikus Dittes 
Scherkl wrote:
 Or you can use an improved opCmp implementation in the 
 compiler, that only add additional runtime cost, if someone is 
 stupid enough to compare signed with unsigned values - but 
 yield the correct result:
For the signed/unsigned comparison problem, specifically - I agree that would be the best solution (and is what D should have done originally). There's nothing "stupid" about doing mixed comparisons correctly, either - I doubt the performance hit would even be measurable in most code. (`checkedint` solves a lot of other problems; the 30% performance hit does *not* come from properly handling signed/unsigned comparisons. A library-level wrapper that only fixed that one problem, and none of the others, could be super fast with proper optimizations.) However, Walter and Andrei already decided that we should just have a warning for unsafe comparisons, instead. Just trying to get the warning implemented has dragged on for years already, so I'm not anxious to go back to square one by arguing for a different solution entirely...
Mar 17 2016
prev sibling next sibling parent tsbockman <thomas.bockman gmail.com> writes:
On Thursday, 17 March 2016 at 09:59:41 UTC, Dominikus Dittes 
Scherkl wrote:
 Or you can use an improved opCmp implementation in the 
 compiler, that only add additional runtime cost, if someone is 
 stupid enough to compare signed with unsigned values - but 
 yield the correct result:
I should also point out that, while it would have been eminently sensible for D to just implement signed/unsigned comparison correctly in the first place - fixing this now would be a silent breaking change. I'm fairly confident that it would cause real-world problems, as I myself can recall trying to write code that intentionally leveraged the current bizarre behavior to eke out a little more speed, in the past. Such code could be trivially fixed with the addition of a `cast(uint)` or the like in the right place, BUT only if someone actually remembered that it needed to be fixed: neither the compiler, nor dfix could automatically detect the rare code that deliberately depends on the current behavior. So, I think Walter and Andrei made the right call by saying to make it a warning. Maybe later though, after a *long* deprecation period, we could fix it to check for negative values like it should.
Mar 17 2016
prev sibling parent reply Basile B <b2.temp gmx.com> writes:
On Thursday, 17 March 2016 at 09:59:41 UTC, Dominikus Dittes 
Scherkl wrote:
 Or you can use an improved opCmp implementation in the 
 compiler, that only add additional runtime cost, [...]
The compiler could statically fix two cases, ALWAYS without runtime cost. I think that FPC does this: operand widening by promoting the unsigned operand to a signed one of a wider type, POC with a template: import std.traits; bool safeIntegralCmp(string op, L, R)(auto ref L lhs, auto ref R rhs) if (isIntegral!R && isIntegral!L) { // safe static if (is(Unqual!L == Unqual!R)) { mixin("return lhs" ~ op ~ "rhs;"); } else { // promote unsigned to bigger signed static if (isSigned!L && R.sizeof < 8) { long widenedRhs = rhs; mixin("return lhs" ~ op ~ "widenedRhs;"); } else static if (isSigned!R && L.sizeof < 8) { long widenedLhs = lhs; mixin("return widened" ~ op ~ "rhs;"); } // not fixable by operand widening else { pragma(msg, "warning, comparing a" ~ L.stringof ~ " with a" ~ R.stringof ~ " may result into wrong results"); mixin("return lhs" ~ op ~ "rhs;"); } } } unittest { int a = -1; uint b; assert(a > b); // wrong result assert(safeIntegralCmp!">"(a,b) == false); // fixed by promotion long aa = -1; ulong bb; assert(aa > bb); // wrong result assert(safeIntegralCmp!">"(aa,bb) == true); // not staticaly fixable, warning } The case where an implicit widening is done a warning could also be emitted (operand bla bla widened).
Mar 17 2016
parent Basile B <b2.temp gmx.com> writes:
On Thursday, 17 March 2016 at 12:35:27 UTC, Basile B wrote:
 import std.traits;

 bool safeIntegralCmp(string op, L, R)(auto ref L lhs, auto ref 
 R rhs)
 if (isIntegral!R && isIntegral!L)
 {
     // safe
     static if (is(Unqual!L == Unqual!R))
     {
         mixin("return lhs" ~ op ~ "rhs;");
     }
     else
     {
         // promote unsigned to bigger signed
         static if (isSigned!L && R.sizeof < 8)
         {
             long widenedRhs = rhs;
             mixin("return lhs" ~ op ~ "widenedRhs;");
         }
         else static if (isSigned!R && L.sizeof < 8)
         {
             long widenedLhs = lhs;
             mixin("return widened" ~ op ~ "rhs;");
         }
         // not fixable by operand widening
         else
         {
             pragma(msg, "warning, comparing a" ~ L.stringof ~ " 
 with a" ~ R.stringof
                 ~ " may result into wrong results");
             mixin("return lhs" ~ op ~ "rhs;");
         }
     }
 }
Obviously I meant to write this: [...] static if (isSigned!L && !isSigned!R && R.sizeof < 8) [...] else static if (isSigned!R && !isSigned!L && L.sizeof < 8) It makes more sense...
Mar 17 2016
prev sibling parent jmh530 <john.michael.hall gmail.com> writes:
On Thursday, 17 March 2016 at 09:13:34 UTC, tsbockman wrote:
 You could use the `DebugInt` wrapper. It aliases to `SafeInt` 
 in debug and unittest mode, to find problems (many, including 
 the specific one in this thread, are detected at compile time). 
 Then, in release mode, it aliases to the built-in types for 
 maximum speed.
Cool. Worth knowing.
Mar 17 2016
prev sibling parent Uranuz <neuranuz gmail.com> writes:
On Wednesday, 16 March 2016 at 16:40:49 UTC, Shachar Shemesh 
wrote:
 Please consider the following program, which is a reduced 
 version of a problem I've spent the entire of today trying to 
 debug:

 import std.stdio;

 void main() {
     enum ulong MAX_VAL = 256;
     long value = -500;

     if( value>MAX_VAL )
         value = MAX_VAL;

     writeln(value);
 }

 People who are marginally familiar with integer promotion will 
 not be surprised to know that the program prints "256". What is 
 surprising to me is that this produced neither error nor 
 warning.

 The comparable program in C++, when compiled with gcc, 
 correctly warns about signed/unsigned comparison (though, to be 
 fair, it seems that clang doesn't).
Yep. Integer promotions in D sucks! I like this example: import std.stdio; void main() { short a = 10; short b = 5; short c = a - b; } It gives error: Error: cannot implicitly convert expression (cast(int)a - cast(int)b) of type int to short Why I can't substract two values of the same type and assign to the variable of the same type directly without casts?! What a nonsense!?
Mar 19 2016