Re: [lmi] Safer is the Yoda style?

[Vadim Zeitlin]

On Sun, 9 Jun 2019 21:17:24 +0000 Greg Chicares <address@hidden> wrote:

GC> When writing an assertion, I experimentally deviated from the "Yoda"
GC> style, to see whether gcc would flag the error. It didn't.

 This is due to this being done inside the assert() macro, which helpfully
adds extra parentheses around its condition, that suppress the
-Wparentheses warning from both gcc and clang. If you instead compile
something like this:
---------------------------------- >8 --------------------------------------
#include <cassert>

int main(int argc, char*[])
{
    assert(argc = -1);
    if ( argc = 17 )                // this is the line 6
        return 1;
    return 0;
}
---------------------------------- >8 --------------------------------------
you do get

yoda_assert.cpp: In function ‘int main(int, char**)’:
yoda_assert.cpp:6:15: warning: suggest parentheses around assignment used as 
truth value [-Wparentheses]
     if ( argc = 17 )
          ~~~~~^~~~

from gcc and 

yoda_assert.cpp:6:15: warning: using the result of an assignment as a condition 
without parentheses [-Wparentheses]
    if ( argc = 17 )
         ~~~~~^~~~
yoda_assert.cpp:6:15: note: place parentheses around the assignment to silence 
this warning
    if ( argc = 17 )
              ^
         (        )
yoda_assert.cpp:6:15: note: use '==' to turn this assignment into an equality 
comparison
    if ( argc = 17 )
              ^
              ==
1 warning generated.

from clang for the wrong use of "=" outside of assert.

 FWIW, MSVC does warn about the use of "=" on both lines.

yoda_assert.cpp(5) : warning C4706: assignment within conditional expression
yoda_assert.cpp(6) : warning C4706: assignment within conditional expression


 While looking into this, I also found this patch

        https://patchwork.ozlabs.org/patch/705844/

which modifies assert macro to avoid hiding these problems, but
unfortunately it doesn't seem to have ever been applied.

 And, for completeness, I also checked if wxASSERT() also suffers from the
same problem and, unfortunately, it does. This is even stranger, however,
as it doesn't add extra parentheses and uses "!(expr)" in its expansion
instead -- and it turns out that this is enough to disable -Wparentheses
from both gcc and clag too (while MSVC still gives it in this case too).
So at the very least, I'm going to apply this change:

https://github.com/wxWidgets/wxWidgets/pull/1345/commits/04435144ac9f27c7958398177741b796987f0daa

if/when it passes the CI builds to let gcc/clang warn about wxASSERT(x=y).


 To summarize, Yoda style is not needed inside normal code, but may,
unfortunately, be useful inside assert() macro as well as other similar
macros if they had been written without explicitly taking this issue into
account as it seems quite simple to suppress such warning accidentally.

 The conclusion I'd like to draw from this is that we shouldn't be using
assert() and modify LMI_ASSERT() in the same way I've modified wxASSERT()
above, i.e. use direct condition instead of its negation.

 Would you agree with that?
VZ

pgpBHtHxwn04F.pgp
Description: PGP signature