19

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?

Bill the Lizard
  • 8,408
  • 9
  • 41
  • 92
DeveloperDon
  • 4,958
  • 1
  • 26
  • 53
  • 41
    Yes. Turn on compiler warnings. Treat warnings as errors and it is not an issue. – Martin York Aug 25 '12 at 06:39
  • possible duplicate of [Doesn't "if (0 == value) ..." do more harm than good?](http://programmers.stackexchange.com/questions/16908/doesnt-if-0-value-do-more-harm-than-good) – Martin York Aug 25 '12 at 07:05
  • 10
    In the given example, the solution is easy. You assign it a boolean value, thus use it as a boolean: `if (confirmExit)`. – Secure Aug 25 '12 at 07:16
  • Since any non-zero value is considered true, comparing a logically Boolean value to 1 (or to `true`) is dangerous. If, for some reason, `confirmExit` has the value 2, then `if (confirmExit == 1)` is incorrect, but `if (confirmExit)` is correct. – Keith Thompson Aug 25 '12 at 09:14
  • 4
    The problem is that the mistake was made by the C language "designers", when they chose to use = for the assignment operator and == for equality comparison. ALGOL used :=, because they specifically wanted to use = for equality comparison, and PASCAL and Ada followed the ALGOL decision. (It is worth noting that, when DoD solicited a C-based entry into the DoD1 bake-off, that eventually yielded Ada, Bell Labs declined, saying that "C was not now and would not ever be robust enough for DoD mission-critical software." One wishes that DoD and the contractors had listened to Bell Labs on this.) – John R. Strohm Aug 25 '12 at 12:57
  • 4
    @John, the choice of symbols isn't the problem, it's the fact that assignments are also an expression that returns the assigned value, allowing either `a = b` or `a == b` inside a conditional. – Karl Bielefeldt Aug 25 '12 at 16:46
  • @KarlBielefeldt, no, expression semantics is not the problem. Consider if(a = b) vs. if (a := b). Using the Algol syntax makes it HARDER to utter the assignment, as it requires two keystrokes, and it makes it EASIER to spot the error. – John R. Strohm Nov 06 '12 at 13:44
  • @JohnR.Strohm This is just half of the cake: if C had := for assign and = for compare you will not mistake comparisons with assignment, but could potentially mistake assignments with comparisons (writing a = b where a:=b is required). You and karl are both right taken together and both wrong taken individually. Of course we can discuss about the "probability" of one mistake respect to the other, but -at the and- it just all about each one's personal habits. – Emilio Garavaglia Nov 06 '12 at 13:56
  • @ShashankJain Well Ok, after my ~10 years of C++ I still happen to make this error once every two years. But of course you're absolutely right in that it's that rare that it doesn't justify any additional measures. In the end the compiler is eager to notify me of this with some warning, anyway (sometimes too eager). – Christian Rau Nov 06 '12 at 14:39
  • @John: int or not, you can still write `if (confirmExit)`, and most expert C programmers would do that. – kevin cline Nov 06 '12 at 16:58
  • @Shashank - I don't think there are any C programmers who have written more than "Hello.c" that have not typed "=" instead of "==" at one time or another -- no matter how experienced. – James Anderson Nov 07 '12 at 06:14

8 Answers8

64

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.

Deduplicator
  • 8,591
  • 5
  • 31
  • 50
Martin York
  • 11,150
  • 2
  • 42
  • 70
  • 2
    Definitely go with the warnings, not the Yoda conditionals - you can forget to do the conditional constant first as easily as you can forget to do ==. – Michael Kohne Aug 25 '12 at 12:00
  • 9
    I got a pleasant surprise that the most voted answer for this question is 'turn compiler warnings into errors'. I was expecting the `if (0 == ret)` horror. – James Aug 25 '12 at 21:17
  • 2
    @James: ... not to mention it will not work for `a == b` !! – Emilio Garavaglia Aug 26 '12 at 21:30
  • 6
    @EmilioGaravaglia: There's an easy workaround for that: Just write `0==a && 0==b || 1==a && 1==b || 2==a && 2==b || ...` (repeat for all possible values). Don't forget the obligatory `...|| 22==a && 22==b || 23==a && 24==b || 25==a && 25==b ||`... error, or the maintenance programmers won't have any fun. – user281377 Nov 06 '12 at 12:16
  • In Visual Studio/Visual C++, add `4706` to the list of warnings to be treated as errors. Look for the option named "Treat Specific Warnings As Errors", it's on the 'Advanced' or 'All Options' section of 'C/C++'. I also suggest adding `4715` for missing return statements. – Dwedit Sep 12 '21 at 20:10
13

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.

Karl Bielefeldt
  • 146,727
  • 38
  • 279
  • 479
6

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.

.\confirmExit\main.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.

DeveloperDon
  • 4,958
  • 1
  • 26
  • 53
  • 9
    Also known as Yoda conditionals: http://wiert.me/2010/05/25/yoda-conditions-from-stackoverflow-new-programming-jargon-you-coined/ and http://stackoverflow.com/questions/8591182/why-if-constant-variable-is-preferred-instead-of-if-variable-constant and http://programmers.stackexchange.com/questions/16908/doesnt-if-0-value-do-more-harm-than-good – Martin York Aug 25 '12 at 07:05
  • 14
    It should be noted that doing things this way will make you fairly unpopular. – riwalk Aug 25 '12 at 18:03
  • 12
    Please don't recommend this practice. – James Aug 25 '12 at 21:18
  • 5
    It also doesn't work if you need to compare two variables to each other. – dan04 Aug 27 '12 at 02:36
  • Some languages actually allow assigning 1 to a new value (yes, I know the Q is a bout c/c++) – Matsemann Nov 07 '12 at 09:32
  • This is just awful. Just tell your compiler not to allow assignments in “if” instead of uglifying your code. – gnasher729 Jun 01 '20 at 06:15
5

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.

Tyler
  • 873
  • 8
  • 16
  • 1
    I disagree. Especially = instead of == can easily slip through by reviewing the code. – Simon Jun 15 '17 at 19:28
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.

octopusgrabbus
  • 699
  • 1
  • 8
  • 20
  • I raise a sceptical eyebrow at any non-beginner programmer who sees a Yoda conditional and doesn't immediately understand it. It's just a flip, not difficult to read, and certainly not as bad as some comments are claiming. – underscore_d Jan 01 '16 at 17:39
  • @underscore_d At most of my employers, assignments within a conditional were frowned on. The thinking was it was better to separate the assignment from the conditional. The reason for being clearer, even at the expense of another line of code was the sustaining engineering factor. I worked in places that had large code bases and a lot of maintenance backlogged. I fully understand someone might want to assign a value, and branch on the condition resulting from the assignment. I see it being done more often in Perl, and, if the intent is clear, I'll follow the design pattern of the author. – octopusgrabbus Jan 02 '16 at 18:54
  • I was thinking about Yoda conditionals alone (like your demo here), not with assignment (as in the OP). I don't mind the former but don't much like the latter either. The only form I deliberately use is `if ( auto myPtr = dynamic_cast(testPtr) ) {` as it avoids keeping a useless `nullptr` in scope if the cast fails - which is presumably the reason C++ has this limited ability to assign within a conditional. For the rest, yeah, a definition should get its own line, I'd say - much easier to see at-a-glance and less prone to assorted slips-of-the-mind. – underscore_d Jan 02 '16 at 20:30
  • @underscore_d Edited answer based on your comments. Good point. – octopusgrabbus Jan 02 '16 at 22:24
1

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

Andrew
  • 2,018
  • 2
  • 16
  • 27
-1

Use const when possible. (Which you should do anyway.)

Solomon Ucko
  • 408
  • 3
  • 10
-2

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.

Inverted Llama
  • 413
  • 2
  • 9
  • 5
    You'd be surprised at how few pointless warnings modern compilers generate. You might not think a warning is important, but in most cases you're just wrong. – Kristof Provost Nov 07 '12 at 12:54
  • Check out the book, "Writing Solid Code" http://www.amazon.com/Writing-Solid-Microsoft-Programming-Series/dp/1556155514. It starts with a great discussion about compilers and how much benefit we could be getting from warning messages and even more extensive static analysis. – DeveloperDon Nov 07 '12 at 14:43
  • @KristofProvost I've managed to get visual studio to give me 10 copies of the exact same warning fro the same line of code, then when this issue was 'fixed' it resulted in 10 identical warnings about the same line of code saying that the original method was better. – Inverted Llama Nov 08 '12 at 11:09
  • @InvertedLlama That's beside the point. You still have to *think* about what you are doing to fix it, which includes not doing something else dangerous. – Caleth Jun 01 '20 at 14:57