Hello Love,
I think you are trying to fix a bug at the wrong end. The boolean types BOOL and BOOLEAN only have, by definition, two valid values: TRUE (aka 1) and FALSE (aka 0). Your 'other' trues (aka 2 to 2^32-1) are illegal values which must NEVER be assigned to a boolean variable. Otherwise you accept to break the checks for TRUE.
Developers MUST be VERY STRICT when assigning values to a boolean variable. They HAVE to make sure that the assigned values are either TRUE or FALSE. IMO other programmers have the right to rely on the fact that a function, which returns a boolean value, only returns the values TRUE or FALSE and NEVER returns any other value. Code like "if (ReadFile(...) == TRUE)" is absolutely OK and MUST work as the programmer expects!
Unfortunately, several month ago, some patches were applied to the Wine code that replaced expressions like "(a < 7) ? TRUE : FALSE" by "(a < 7)". These patches could be the origin of some bugs and should be reverted from the Wine codebase.
Regards, Eric
Am 12.11.2014 10:48, schrieb Love Nystrom:
Grep'ing for [ \t]*==[ \t]*TRUE and [ \t]*!=[ \t]*TRUE revealed some 400 matches.. That's *400 potential malfunctions begging to happen*, as previously concluded.
If you *must*, for some obscure reason, code an explicit truth-value comparison, for God's sake make it (boolVal != FALSE) or (boolVal == FALSE), which is safe, because a BOOL has 2^32-2 TRUE values !!!
However, the more efficient "if ( boolVal )" and "if ( !boolVal )" ought to be *mandatory*.
I do hope nobody will challenge that "if ( boolVal )" equals "if ( boolVal != FALSE )", and does *not* equal "if ( boolVal == TRUE )", when boolVal is BOOL or BOOLEAN...
I've patched all those potential errors against the current trunk. In most cases a simple removal of "== TRUE" was sufficient, however in asserts I replaced it with "!= FALSE", since that may be clearer when triggered. The only places I let it pass was in pure debug strings and comments.
As this is a *fairly extensive patch*, I would very much appreciate if a *prioritized regression test* could be run by you guys who do such things, since this may actually fix some "mysterious" malfunctions, or introduce bugs that did not trigger in my alpha test.
My own alpha test was limited to building and installing it on VMware Player 6, and concluding that "it appears to run without obvious malfunctions". *Actually, when compared to a pre-patch build, one "mysterious" crash disappeared!*
The patch has been submitted as bug CORE-8799, and is also included inline in this post.
Best Regards // Love