In C and C++, it is very easy to write the following code with a serious error.
char responseChar = getchar();
int confirmExit = 'y' == tolower(responseChar);
if (confirmExit = 1)
{
exit(0);
}
The error is that the if statement should have been:
if (confirmExit == 1)
As coded, it will exit every time, because the assignment of the confirmExit variable occurs, then confirmExit is used as the result of the expression.
Are there good ways to prevent this kind of error?
11
The best technique is to increase the warning level of your compiler.
It will then warn you about potential assignment in the if conditional.
Make sure you compile your code with zero warnings (which you should be doing anyway). If you want to be pedantic then set your compiler to treat warnings as errors.
Using Yoda conditionals (putting the constant on the left hand side) was another technique that was popular about a decade ago. But they make the code harder to read (and thus maintain because of the unnatural way they read (unless you are Yoda)) and provide no greater benefit than increasing the warning level (which also has extra benefits of more warnings).
Warnings are really logical errors in the code and should be corrected.
5
You could always do something radical like testing your software. I don’t even mean automated unit tests, just the tests every single experienced developer does out of habit by running his new code twice, once confirming the exit and once not. That’s the reason most programmers consider it a non-issue.
4
A traditional way to prevent the incorrect use of assignments within expression is to place the constant on the left and the variable on the right.
if (confirmExit = 1) // Unsafe
if (1=confirmExit) // Safe and detected at compile time.
The compiler will report an error for the illegal assignment to a constant similar to the following.
.confirmExitmain.cpp:15: error: C2106: '=' : left operand must be l-value
The revised if condition would be:
if (1==confirmExit)
As shown by comments below, this is considered by many to be an inappropriate method.
6
I agree with everyone saying “compiler warnings” but I want to add another technique: Code reviews. If you have a policy of reviewing all code that gets committed, preferably before it’s committed, then it’s likely this kind of thing will be caught during review.
2
First, raising your warning levels never hurts.
If you do not want your conditional to test the result of an assignment within the if statement itself, then having worked with a lot of C and C++ programmers over the years, and having never heard that comparing the constant first if(1 == val) was a bad thing, you could try that construct.
If your project leader approves of your doing this, don’t worry about what other people think. The real proof is whether you or someone else can make sense of your code months and years from now.
If you intention was to test the result of an assignment, however, then using higher warnings might [probably would have] caught the assignment to a constant.
4
Late to the party as ever, but Static Code Analysis is the key here
Most IDEs now provide SCA over and above the syntactic check of the compiler, and other tools are available, including those that implement the MISRA (*) and/or CERT-C guidelines.
Declaration: I am part of the MISRA C working group, but I’m posting in a personal capacity. I’m also independent of any tool vendor
Use const when possible. (Which you should do anyway.)
Just use left hand assignment, compiler warnings can help but you have to make sure you get the right level otherwise you’ll either be swamped with pointless warnings or won’t be told ones you want to see.
4