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?

Improve this question
11
  • 41
    Yes. Turn on compiler warnings. Treat warnings as errors and it is not an issue.
    – Martin York
    Aug 25, 2012 at 6:39
  • possible duplicate of Doesn't "if (0 == value) ..." do more harm than good?
    – Martin York
    Aug 25, 2012 at 7: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, 2012 at 7:16
  • 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, 2012 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, 2012 at 16:46

8 Answers 8

Reset to default
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.

Improve this answer
5
  • 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, 2012 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, 2012 at 21:17
  • 2
    @James: ... not to mention it will not work for a == b !!
    – Emilio Garavaglia
    Aug 26, 2012 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 6, 2012 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, 2021 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.

Improve this answer
4
  • 1
    Easy to do when the behaviour of your program is completely deterministic.
    – James
    Aug 25, 2012 at 21:19
  • 4
    This particular issue can be hard to test for. I've seen people bitten by rc=MethodThatRarelyFails(); if(rc = SUCCESS){ more than once, especially if the method fails only in conditions that are hard to test for.
    – Gort the Robot
    Aug 25, 2012 at 23:50
  • 4
    @StevenBurnap: That's what mock objects are for. Proper testing includes testing of failure modes.
    – Jan Hudec
    Nov 6, 2012 at 13:47
  • 1
    This is the correct answer.
    – Lightness Races in Orbit
    Jan 3, 2016 at 23:23
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.

Improve this answer
6
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.

Improve this answer
1
  • 1
    I disagree. Especially = instead of == can easily slip through by reviewing the code.
    – Simon
    Jun 15, 2017 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.

Improve this answer
4
  • 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 1, 2016 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 2, 2016 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<some_ptr>(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 2, 2016 at 20:30
  • @underscore_d Edited answer based on your comments. Good point.
    – octopusgrabbus
    Jan 2, 2016 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

Improve this answer
-1

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

Improve this answer
-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.

Improve this answer
4
  • 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 7, 2012 at 12:54
  • Check out the book, "Writing Solid Code" amazon.com/Writing-Solid-Microsoft-Programming-Series/dp/…. 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 7, 2012 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 8, 2012 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 1, 2020 at 14:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.