15

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?

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

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.

  • 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
  • 8
    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 6 '12 at 12:16
10

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.

  • 1
    Easy to do when the behaviour of your program is completely deterministic. – James Aug 25 '12 at 21:19
  • 3
    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. – Steven Burnap Aug 25 '12 at 23:50
  • 3
    @StevenBurnap: That's what mock objects are for. Proper testing includes testing of failure modes. – Jan Hudec Nov 6 '12 at 13:47
  • This is the correct answer. – Lightness Races in Orbit Jan 3 '16 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.

4

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.

  • 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.

  • 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 '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 2 '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<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 '16 at 20:30
  • @underscore_d Edited answer based on your comments. Good point. – octopusgrabbus Jan 2 '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

-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.

  • 4
    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 '12 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 '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 8 '12 at 11:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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