www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Yet another slap on the hand by implicit bool to int conversions

reply Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
So I've had a pretty fun bug today when porting some code over to D.
When I port C code to D, first I do a one-to-one translation and make
sure everything works before I try to make use of any D features.

Here's a function snippet, try to figure out what's wrong with the
code (don't worry about type definitions or external variables yet):

http://codepad.org/gYNxJ8a5

Ok that's quite a bit of code. Let's cut to the chase:

if (mmioRead(hmmio, cast(LPSTR)&drum, DRUM.sizeof != DRUM.sizeof))
{
    mmioClose(hmmio, 0);
    return szErrorCannotRead;
}

Here goes: The first argument to "mmioRead" is the already opened file
handle, the second is the instance variable to write to (a struct
variable of type DRUM), and the third argument is the number of bytes
to read (an integer).

On success, the function will return the number of bytes read. We
compare this to the size of the DRUM structure, and if the sizes are
not equal then something has gone wrong. I've added a writeln() call
inside the if body to verify that the if statement body is never
executed.

Yet, I have no data in the "drum" variable after the call! What on
earth did go wrong?

Let's take a closer look at the call:

if (mmioRead(hmmio, cast(LPSTR)&drum, DRUM.sizeof != DRUM.sizeof))

Look at that nasty parenthesis at the end, it's not in the right
position at all! It should be:

if (mmioRead(hmmio, cast(LPSTR)&drum, DRUM.sizeof) != DRUM.sizeof)

So, what happened with the original call?

This expression: DRUM.sizeof != DRUM.sizeof
was evaluated and converted to this boolean: false
which was implicitly converted to an int: 0
which means mmioRead has read 0 bytes, and remember how this function
returns the number of bytes it has read? Well it returns 0.
0 in turn is implicitly converted to a bool, and the if body is not executed.

Btw, this is all compiling with all the warnings turned on. If I had
at least a warning emitted from the compiler I could have spotted the
bug sooner and not waste time due to silly (admittedly my) typos like
these.
Jun 19 2011
next sibling parent reply "Vladimir Panteleev" <vladimir thecybershadow.net> writes:
On Sun, 19 Jun 2011 23:13:06 +0300, Andrej Mitrovic  
<andrej.mitrovich gmail.com> wrote:

 So I've had a pretty fun bug today when porting some code over to D.
 When I port C code to D, first I do a one-to-one translation and make
 sure everything works before I try to make use of any D features.

How would you expect D to deal with types such as BOOL (which is a 32-bit type)? -- Best regards, Vladimir mailto:vladimir thecybershadow.net
Jun 19 2011
parent reply Mehrdad <wfunction hotmail.com> writes:
On 6/19/2011 2:24 PM, Vladimir Panteleev wrote:
 On Sun, 19 Jun 2011 23:13:06 +0300, Andrej Mitrovic 
 <andrej.mitrovich gmail.com> wrote:

 So I've had a pretty fun bug today when porting some code over to D.
 When I port C code to D, first I do a one-to-one translation and make
 sure everything works before I try to make use of any D features.

How would you expect D to deal with types such as BOOL (which is a 32-bit type)?

How do you deal with BOOL? By emitting a **Warning** instead of an error, which is only turned on with an appropriate compiler option (say, higher verbosity). Simple as that. :) Mehrdad
Jun 19 2011
parent bearophile <bearophileHUGS lycos.com> writes:
Mehrdad:

 How do you deal with BOOL?

D typedef was meant for this. Until there is something (probably library-based) to replace it, there are structs with alias this. Bye, bearophile
Jun 19 2011
prev sibling next sibling parent Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
On 6/19/11, Vladimir Panteleev <vladimir thecybershadow.net> wrote:
 On Sun, 19 Jun 2011 23:13:06 +0300, Andrej Mitrovic
 <andrej.mitrovich gmail.com> wrote:

 So I've had a pretty fun bug today when porting some code over to D.
 When I port C code to D, first I do a one-to-one translation and make
 sure everything works before I try to make use of any D features.

How would you expect D to deal with types such as BOOL (which is a 32-bit type)?

I'm not sure what you're going for? alias int BOOL; enum : BOOL { FALSE = 0, TRUE = 1 } void main() { bool value = getTrue(); } BOOL getTrue() { return TRUE; } Error: cannot implicitly convert expression (getTrue()) of type int to bool My problem was implicit *D bool* conversion to D int types. Not the windows bool type which is an int.
Jun 19 2011
prev sibling next sibling parent "Vladimir Panteleev" <vladimir thecybershadow.net> writes:
On Mon, 20 Jun 2011 00:32:38 +0300, Andrej Mitrovic  
<andrej.mitrovich gmail.com> wrote:

 I'm not sure what you're going for?

 alias int BOOL;
 enum : BOOL {

And now you can't pass D boolean expressions to Win32 APIs without a cast or a "?:". -- Best regards, Vladimir mailto:vladimir thecybershadow.net
Jun 19 2011
prev sibling next sibling parent reply Timon Gehr <timon.gehr gmx.ch> writes:
Andrej Mitrovic wrote:
 So I've had a pretty fun bug today when porting some code over to D.
 When I port C code to D, first I do a one-to-one translation and make
 sure everything works before I try to make use of any D features.

 Here's a function snippet, try to figure out what's wrong with the
 code (don't worry about type definitions or external variables yet):

 http://codepad.org/gYNxJ8a5

 Ok that's quite a bit of code. Let's cut to the chase:

 if (mmioRead(hmmio, cast(LPSTR)&drum, DRUM.sizeof != DRUM.sizeof))
 {
     mmioClose(hmmio, 0);
     return szErrorCannotRead;
 }
 [snip.]

Maybe DMD could warn on nonsense of the form x != x. Timon
Jun 19 2011
next sibling parent reply Peter Alexander <peter.alexander.au gmail.com> writes:
On 19/06/11 10:51 PM, Timon Gehr wrote:
 Andrej Mitrovic wrote:
 So I've had a pretty fun bug today when porting some code over to D.
 When I port C code to D, first I do a one-to-one translation and make
 sure everything works before I try to make use of any D features.

 Here's a function snippet, try to figure out what's wrong with the
 code (don't worry about type definitions or external variables yet):

 http://codepad.org/gYNxJ8a5

 Ok that's quite a bit of code. Let's cut to the chase:

 if (mmioRead(hmmio, cast(LPSTR)&drum, DRUM.sizeof != DRUM.sizeof))
 {
      mmioClose(hmmio, 0);
      return szErrorCannotRead;
 }
 [snip.]

Maybe DMD could warn on nonsense of the form x != x. Timon

That would help in this particular case, but not in general.
Jun 19 2011
parent reply Timon Gehr <timon.gehr gmx.ch> writes:
Peter Alexander wrote:
 On 19/06/11 10:51 PM, Timon Gehr wrote:
 Andrej Mitrovic wrote:
 So I've had a pretty fun bug today when porting some code over to D.
 When I port C code to D, first I do a one-to-one translation and make
 sure everything works before I try to make use of any D features.

 Here's a function snippet, try to figure out what's wrong with the
 code (don't worry about type definitions or external variables yet):

 http://codepad.org/gYNxJ8a5

 Ok that's quite a bit of code. Let's cut to the chase:

 if (mmioRead(hmmio, cast(LPSTR)&drum, DRUM.sizeof != DRUM.sizeof))
 {
      mmioClose(hmmio, 0);
      return szErrorCannotRead;
 }
 [snip.]

Maybe DMD could warn on nonsense of the form x != x. Timon

That would help in this particular case, but not in general.

I claim that in this particular case the x != x was more of a problem than the implicit conversions. Implicit conversions from boolean to int don't hurt in general. But they are also not generally useful, just if you want to use arithmetics on bools: bool b; int i=b; // you seldom want to do this int month; int daysinmonth = month==1?28:30+((m&1)==(m>6)); // kinda neat, implicit conversions in 2 places A possible resolution could be to just disallow direct implicit conversion but allow bools in arithmetic expressions. That would be special casing though. Timon
Jun 19 2011
parent Timon Gehr <timon.gehr gmx.ch> writes:
Sry should have been

int daysinmonth = month==1?28:30+((month&1)==(month>6));

obv.

Cheers,
-Timon
Jun 19 2011
prev sibling parent reply bearophile <bearophileHUGS lycos.com> writes:
Timon Gehr:

 Maybe DMD could warn on nonsense of the form x != x.

Right. This thread is very good food for this enhancement request of mine: http://d.puremagic.com/issues/show_bug.cgi?id=5540 Vote for this enhancement :-) ---------------------- Peter Alexander:
 That would help in this particular case, but not in general.

Experience (there are even articles written on this, with statistics) shows this is a a common class of bug, and for a compiler it's easy to catch such bugs. So while not being very general as you want, it's general enough to justify a compiler test for such things. I hope D will catch similar things. Bye, bearophile
Jun 19 2011
next sibling parent reply Daniel Gibson <metalcaedes gmail.com> writes:
Am 20.06.2011 14:47, schrieb Steven Schveighoffer:
 On Sun, 19 Jun 2011 19:42:22 -0400, bearophile
 <bearophileHUGS lycos.com> wrote:
 
 Timon Gehr:

 Maybe DMD could warn on nonsense of the form x != x.

Right. This thread is very good food for this enhancement request of mine: http://d.puremagic.com/issues/show_bug.cgi?id=5540 Vote for this enhancement :-)

I don't think this is a good idea. Generic code could result in an error where there shouldn't be. For example: int foo(T)() { if(T.sizeof == char.sizeof) ... } Would this fail to compile where T == char? What about if T == ubyte? Generic programming sometimes results in silly code that is perfectly acceptable as generic code, and we need to take this into account before making decisions assuming a person is writing the code. -Steve

It probably makes more sense to use static if in that case - and static if could be an exception for these rules. Cheers, - Daniel
Jun 20 2011
parent reply Daniel Gibson <metalcaedes gmail.com> writes:
Am 20.06.2011 15:31, schrieb Steven Schveighoffer:
 On Mon, 20 Jun 2011 09:17:56 -0400, Daniel Gibson
 <metalcaedes gmail.com> wrote:
 
 Am 20.06.2011 14:47, schrieb Steven Schveighoffer:
 On Sun, 19 Jun 2011 19:42:22 -0400, bearophile
 <bearophileHUGS lycos.com> wrote:

 Timon Gehr:

 Maybe DMD could warn on nonsense of the form x != x.

Right. This thread is very good food for this enhancement request of mine: http://d.puremagic.com/issues/show_bug.cgi?id=5540 Vote for this enhancement :-)

I don't think this is a good idea. Generic code could result in an error where there shouldn't be. For example: int foo(T)() { if(T.sizeof == char.sizeof) ... } Would this fail to compile where T == char? What about if T == ubyte? Generic programming sometimes results in silly code that is perfectly acceptable as generic code, and we need to take this into account before making decisions assuming a person is writing the code. -Steve

It probably makes more sense to use static if in that case - and static if could be an exception for these rules.

static if has different semantics than if (it doesn't create a scope), so maybe I want to use if. But even so, static if is just as likely to contain these bugs as a normal if. Both use an expression to determine whether the if should run or not, and there are quite a few constructs that can be used in a static if expression.

Disallowing some things in if that are allowed in static if at least catch most bugs of this kind because normal if is more common. Maybe, as an alternative, "nonsensical" expressions could be allowed if template parameters are involved. Don't know how hard it is to implement this, though. Another alternative: nonsensical expressions need to be enclosed by an extra pair of parenthesis. I think I have seen this for cases like if( (x=foo()) ) {}, meaning if( foo() != 0), but x is assigned that value at the same time (so you could as well write "if( (x=foo()) != 0)", but I don't remember in what language (D doesn't allow it). Maybe it was a warning of g++? Cheers, - Daniel
Jun 20 2011
parent bearophile <bearophileHUGS lycos.com> writes:
Steven Schveighoffer:

 In order to have a fix for something like this, you need the error to be  
 near 100% invalid.  Like nobody ever writes this as *valid* code:
 
 if(cond);
 
 no matter what cond is.

My enhancement request was about redundancies in the code, that sometimes hide implicit errors, they can't be 'near 100% invalid'. In this case I am not looking for explicit errors, some/many of the redundancies aren't bugs. Bye, bearophile
Jun 20 2011
prev sibling parent bearophile <bearophileHUGS lycos.com> writes:
Steven Schveighoffer:

 I don't think this is a good idea.  Generic code could result in an error  
 where there shouldn't be.  For example:
 
 int foo(T)()
 {
     if(T.sizeof == char.sizeof)
       ...
 }
 
 Would this fail to compile where T == char?  What about if T == ubyte?

I was thinking more about a warning than an error. But you are right, my enhancement request 5540 seems premature, I/we have to think some more about it. Bye and sorry, bearophile
Jun 20 2011
prev sibling next sibling parent "Steven Schveighoffer" <schveiguy yahoo.com> writes:
On Sun, 19 Jun 2011 19:42:22 -0400, bearophile <bearophileHUGS lycos.com>  
wrote:

 Timon Gehr:

 Maybe DMD could warn on nonsense of the form x != x.

Right. This thread is very good food for this enhancement request of mine: http://d.puremagic.com/issues/show_bug.cgi?id=5540 Vote for this enhancement :-)

I don't think this is a good idea. Generic code could result in an error where there shouldn't be. For example: int foo(T)() { if(T.sizeof == char.sizeof) ... } Would this fail to compile where T == char? What about if T == ubyte? Generic programming sometimes results in silly code that is perfectly acceptable as generic code, and we need to take this into account before making decisions assuming a person is writing the code. -Steve
Jun 20 2011
prev sibling next sibling parent "Steven Schveighoffer" <schveiguy yahoo.com> writes:
On Mon, 20 Jun 2011 09:17:56 -0400, Daniel Gibson <metalcaedes gmail.com>  
wrote:

 Am 20.06.2011 14:47, schrieb Steven Schveighoffer:
 On Sun, 19 Jun 2011 19:42:22 -0400, bearophile
 <bearophileHUGS lycos.com> wrote:

 Timon Gehr:

 Maybe DMD could warn on nonsense of the form x != x.

Right. This thread is very good food for this enhancement request of mine: http://d.puremagic.com/issues/show_bug.cgi?id=5540 Vote for this enhancement :-)

I don't think this is a good idea. Generic code could result in an error where there shouldn't be. For example: int foo(T)() { if(T.sizeof == char.sizeof) ... } Would this fail to compile where T == char? What about if T == ubyte? Generic programming sometimes results in silly code that is perfectly acceptable as generic code, and we need to take this into account before making decisions assuming a person is writing the code. -Steve

It probably makes more sense to use static if in that case - and static if could be an exception for these rules.

static if has different semantics than if (it doesn't create a scope), so maybe I want to use if. But even so, static if is just as likely to contain these bugs as a normal if. Both use an expression to determine whether the if should run or not, and there are quite a few constructs that can be used in a static if expression. -Steve
Jun 20 2011
prev sibling next sibling parent "Steven Schveighoffer" <schveiguy yahoo.com> writes:
On Mon, 20 Jun 2011 09:42:45 -0400, Daniel Gibson <metalcaedes gmail.com>  
wrote:

 Am 20.06.2011 15:31, schrieb Steven Schveighoffer:
 On Mon, 20 Jun 2011 09:17:56 -0400, Daniel Gibson
 <metalcaedes gmail.com> wrote:

 Am 20.06.2011 14:47, schrieb Steven Schveighoffer:
 On Sun, 19 Jun 2011 19:42:22 -0400, bearophile
 <bearophileHUGS lycos.com> wrote:

 Timon Gehr:

 Maybe DMD could warn on nonsense of the form x != x.

Right. This thread is very good food for this enhancement request of mine: http://d.puremagic.com/issues/show_bug.cgi?id=5540 Vote for this enhancement :-)

I don't think this is a good idea. Generic code could result in an error where there shouldn't be. For example: int foo(T)() { if(T.sizeof == char.sizeof) ... } Would this fail to compile where T == char? What about if T == ubyte? Generic programming sometimes results in silly code that is perfectly acceptable as generic code, and we need to take this into account before making decisions assuming a person is writing the code. -Steve

It probably makes more sense to use static if in that case - and static if could be an exception for these rules.

static if has different semantics than if (it doesn't create a scope), so maybe I want to use if. But even so, static if is just as likely to contain these bugs as a normal if. Both use an expression to determine whether the if should run or not, and there are quite a few constructs that can be used in a static if expression.

Disallowing some things in if that are allowed in static if at least catch most bugs of this kind because normal if is more common. Maybe, as an alternative, "nonsensical" expressions could be allowed if template parameters are involved. Don't know how hard it is to implement this, though. Another alternative: nonsensical expressions need to be enclosed by an extra pair of parenthesis. I think I have seen this for cases like if( (x=foo()) ) {}, meaning if( foo() != 0), but x is assigned that value at the same time (so you could as well write "if( (x=foo()) != 0)", but I don't remember in what language (D doesn't allow it). Maybe it was a warning of g++?

Another point, let's say you know the size of an int is 4. And you write something like: if(read(stream, ptr, int.sizeof == 4)) where you meant to write: if(read(stream, ptr, int.sizeof) == 4) How can the compiler "catch" this? It's the equivalent of int.sizeof == int.sizeof, but it's not exactly written that way. Another example: if(read(stream, &x, x.sizeof == int.sizeof)) // x is of type int It just seems too arbitrary to me to say exact_expression == exact_expression is invalid. It doesn't solve all the cases where you mistype something, and it can throw errors that are nuisances. In order to have a fix for something like this, you need the error to be near 100% invalid. Like nobody ever writes this as *valid* code: if(cond); no matter what cond is. -Steve
Jun 20 2011
prev sibling parent "Steven Schveighoffer" <schveiguy yahoo.com> writes:
On Mon, 20 Jun 2011 11:10:58 -0400, bearophile <bearophileHUGS lycos.com>  
wrote:

 Steven Schveighoffer:

 In order to have a fix for something like this, you need the error to be
 near 100% invalid.  Like nobody ever writes this as *valid* code:

 if(cond);

 no matter what cond is.

My enhancement request was about redundancies in the code, that sometimes hide implicit errors, they can't be 'near 100% invalid'. In this case I am not looking for explicit errors, some/many of the redundancies aren't bugs.

What I'm saying is, if the compiler errors out when a good percentage of the cases are valid, it will become a nuisance error, and continually ignored/worked around. We do not want nuisance errors. Even if you make it a error only with -w, I think people will just stop using -w. It needs to be: a) almost always an error when it is encountered b) very easily worked around, but with code that isn't common. I think with the extra parentheses idea, b is satisfied, but a is not. I can't say that there isn't a case where a is not satisfied by some specific criteria, but I haven't seen it yet. -Steve
Jun 20 2011