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
next sibling parent reply 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
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 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)?


I agree, this is a ridiculous bug to have to spend so much time for.

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 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
parent reply "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
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 "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
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 reply "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
parent reply 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
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
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 parent Kagamin <spam here.lot> writes:
Andrej Mitrovic Wrote:


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


How would you catch this bug:

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

If you didn't notice what you converted the code to, you probably forgot what's
going on in this code. It has not that many braces to get lost in them.
Jun 20 2011