55

I recently just lost some time figuring out a bug in my code which was caused by a typo:

if (a=b)

instead of:

if (a==b)

I was wondering if there is any particular case you would want to assign a value to a variable in a if statement, or if not, why doesn't the compiler throw a warning or an error?

16
  • 4
    I depends, my compiler throws a warning when a = b
    – user1944441
    Commented Jul 16, 2013 at 16:03
  • 3
    The compiler doesn't issue a diagnostic because it is not mandated to do so. It is perfectly valid C++ code.
    – Alok Save
    Commented Jul 16, 2013 at 16:05
  • 7
    Most compilers will warn about this, as long as you enable the warning. Commented Jul 16, 2013 at 16:07
  • 3
    I vote against treating this as a duplicate of stackoverflow.com/q/151850/2932052, because this is an actual C/C++ question that arises often whereas the other is very vague and language-agnostic. Maybe it's really only a C/C++ problem? In this case the other should get the tag and this question should be treated as duplicate.
    – Wolf
    Commented Apr 23, 2019 at 9:27
  • 2
    Does this answer your question? Inadvertent use of = instead of ==
    – outis
    Commented Jun 30, 2022 at 5:42

9 Answers 9

69
if (Derived* derived = dynamic_cast<Derived*>(base)) {
   // do stuff with `derived`
}

Though this is oft cited as an anti-pattern ("use virtual dispatch!"), sometimes the Derived type has functionality that the Base simply does not (and, consequently, distinct functions), and this is a good way to switch on that semantic difference.

19
  • 17
    The anti-pattern is putting a definition in the condition. It's a sure recipe for unreadable and unmaintainable code. Commented Jul 16, 2013 at 16:25
  • 76
    Putting the definition in the condition tightens the scope so that you don't accidentally refer to derived on the next line after the if-statement. It's the same reason we use for (int i = 0... instead of int i; for (i = 0; .... Commented Jul 16, 2013 at 16:36
  • 62
    @JamesKanze - Others find it increases readability and maintainability, partly by minimizing the scope of the variable. "To avoid accidental misuse of a variable, it is usually a good idea to introduce the variable into the smallest scope possible. In particular, it is usually best to delay the definition of a variable until one can give it an initial value ... One of the most elegant applications of these two principles is to declare a variable in a conditional." -- Stroustrup, "The C++ Programming Language." Commented Jul 16, 2013 at 16:40
  • 13
    @James: Nobody in my team has had a problem either reading or maintaining the code, in the ten years our codebase has taken this approach. Commented Jul 16, 2013 at 17:42
  • 15
    Funny thing about the scope of the variable when putting a definition in the condition is that the variable is also accessible in the else-clause as well. if (T t = foo()) { dosomething(t); } else { dosomethingelse(t); }
    – dalle
    Commented Jul 18, 2013 at 8:57
53

Here is some history on the syntax in question.

In classical C, error handling was frequently done by writing something like:

int error;
...
if(error = foo()) {
    printf("An error occurred: %s\nBailing out.\n", strerror(error));
    abort();
}

Or, whenever there was a function call that might return a null pointer, the idiom was used the other way round:

Bar* myBar;
... // In old C variables had to be declared at the start of the scope
if(myBar = getBar()) {
    // Do something with myBar
}

However, this syntax is dangerously close to

if(myValue == bar()) ...

which is why many people consider the assignment inside a condition bad style, and compilers started to warn about it (at least with -Wall). Some compilers allow avoiding this warning by adding an extra set of parentheses:

if((myBar = getBar())) {  // It tells the compiler: Yes, I really want to do that assignment!

However, this is ugly and somewhat of a hack, so it's better avoid writing such code.

Then C99 came around, allowing you to mix definitions and statements, so many developers would frequently write something like

Bar* myBar = getBar();
if(myBar) {

which does feel awkward. This is why the newest standard allows definitions inside conditions, to provide a short, elegant way to do this:

if(Bar* myBar = getBar()) {

There isn't any danger in this statement anymore. You explicitly give the variable a type, obviously wanting it to be initialized. It also avoids the extra line to define the variable, which is nice. But most importantly, the compiler can now easily catch this sort of bug:

if(Bar* myBar = getBar()) {
    ...
}
foo(myBar->baz);  // Compiler error
myBar->foo();     // Compiler error

Without the variable definition inside the if statement, this condition would not be detectable.

To make a long answer short: The syntax in you question is the product of old C's simplicity and power, but it is evil, so compilers can warn about it. Since it is also a very useful way to express a common problem, there is now a very concise, bug robust way to achieve the same behavior. And there are a lot of good, possible uses for it.

4
  • 1
    But adding extra parenthesis is not "bug robust": approxion.com/a-gcc-compiler-mistake The MISRA-compliant code if ((a == b) && (c = d)) won't generate a warning or error because of GCC's "feature". Instead of "bug robust" it's perniciously buggy, You think using -Wall -Werror protects you from such bugs, but it actually doesn't. Commented Jun 6, 2021 at 12:12
  • 2
    @AndrewHenle Well, that part about assignment in extra brackets was only to illustrate what has been done many times in real existing code. Of course, the extra parentheses are only a hack. Of course, they look ugly. Of course, this is just an extra reason to prefer the C++ variable declaration over a C assignment in an if() statement. You may question the sensibility of GCC's warning behavior, but that has no impact on my answer at all. Commented Jun 6, 2021 at 13:16
  • 1
    The issue isn't with your answer - I think it's about the best possible factual discussion of the history of the "assignment in an if-statement" style came about. The problem is your answer was referred by someone who utterly missed your characterization of GCC's hack and actually used your answer as a defense of that hack. I was pointing out how GCC's hack actually opens the door to even more subtle bugs when more complex code is involved. That evil aspect of the GCC "extra parentheses" hack seems to be utterly missed by proponents of cramming assignments into if statements. Commented Jun 6, 2021 at 16:12
  • 3
    @AndrewHenle Thanks for the explanation. It gets me thinking that it might be beneficial to point out that this is a hack, and that I discourage using it. Commented Jun 6, 2021 at 18:44
21

The assignment operator returns the value of the assigned value. So, I might use it in a situation like this:

if (x = getMyNumber())

I assign x to be the value returned by getMyNumber and I check if it's not zero.

Avoid doing that. I gave you an example just to help you understand this.

To avoid such bugs up to some extent, one should write the if condition as if(NULL == ptr) instead of if (ptr == NULL). Because when you misspell the equality check operator == as operator =, the compiler will throw an lvalue error with if (NULL = ptr), but if (res = NULL) passed by the compiler (which is not what you mean) and remain a bug in code for runtime.

One should also read Criticism regarding this kind of code.

11
  • 14
    I want to learn/improve myself, please mention why you downvote.
    – Maroun
    Commented Jul 16, 2013 at 16:16
  • 2
    I frequently use if(fp = fopen("file", "w")){ // file not open} else{ // file processing } Commented Jul 16, 2013 at 19:00
  • 18
    Reversing the operands of an equality comparison, such as writing (NULL == ptr) rather than (ptr == NULL), is controversial. Personally, I hate it; I find it jarring, and I have to mentally re-reverse it to understand it. (Others obviously don't have that issue). The practice is referred to as "Yoda conditions". Most compilers can be persuaded to warn about assignments in conditions anyway. Commented Jul 16, 2013 at 19:30
  • 1
    I think its nice to know the trick! I added as an information, Some people may like it. That is why its just a suggestion. Commented Jul 16, 2013 at 20:01
  • 1
    @LilianA.Moraru For some it is helpful. That's why it's a suggestion, if you don't like it you don't have to use it.
    – Maroun
    Commented Oct 17, 2017 at 8:36
15

why doesn't the compiler throw a warning

Some compilers will generate warnings for suspicious assignments in a conditional expression, though you usually have to enable the warning explicitly.

For example, in Visual C++, you have to enable C4706 (or level 4 warnings in general). I generally turn on as many warnings as I can and make the code more explicit in order to avoid false positives. For example, if I really wanted to do this:

if (x = Foo()) { ... }

Then I'd write it as:

if ((x = Foo()) != 0) { ... }

The compiler sees the explicit test and assumes that the assignment was intentional, so you don't get a false positive warning here.

The only drawback with this approach is that you can't use it when the variable is declared in the condition. That is, you cannot rewrite:

if (int x = Foo()) { ... }

as

if ((int x = Foo()) != 0) { ... }

Syntactically, that doesn't work. So you either have to disable the warning, or compromise on how tightly you scope x.

C++17 added the ability to have an init-statement in the condition for an if statement (p0305r1), which solves this problem nicely (for kind of comparison, not just != 0).

if (x = Foo(); x != 0) { ... }

Furthermore, if you want, you can limit the scope of x to just the if statement:

if (int x = Foo(); x != 0) { /* x in scope */ ... }
// x out of scope
5
  • The explicit comparison if((x = Foo()) != 0) is unnecessary: adding an extra pair of parentheses if((x = Foo())) will silence the warning just as well. Also, there is a big difference between if(x = Foo()) and if(int x = Foo()), the later clearly states that you want to do an initialization, not a comparison, so no compiler in their right mind would want to throw a warning on this. Commented Aug 26, 2013 at 17:35
  • 2
    To be fair, you don't have to compromise how tightly you scope x if you are willing to surround your block with extra braces. But I wager this is not usually worth it. Commented Aug 26, 2013 at 18:30
  • 2
    @cmaster: Explicit comparison is necessary if you want to check something other than != 0. Explicit comparison is also clearer than an extra set of parentheses. Commented Aug 26, 2013 at 20:33
  • 1
    @AdrianMcCarthy: Explicit comparison also requires that extra set of parentheses (due to operator precedence). So the addition of ` != 0` is always just extra code. Of course, you are right that explicit comparison can compare to anything. If you are comparing against 0, however, I heartily disagree with you: This is such a common idiom that I expect the shorter version in that case, so that the extra ` != 0` has the effect of a small pothole to me... Commented Aug 26, 2013 at 20:53
  • 1
    @cmaster: That may be the case for you. But other programmers may be confused and think the extra parentheses are superfluous. They could delete them (which might not bother the compiler if they haven't enabled the same warning). Then someone will come along and think the assignment was supposed to be ==, and now you've got two bugs. The explicit comparison makes it obvious clear why the extra parentheses are there and, more importantly, that the assignment was not a typo. Commented Mar 27, 2019 at 12:11
13

In C++17, one can use:

if (<initialize> ; <conditional_expression>) { <body> }

Similar to a for loop iterator initializer.

Here is an example:

if (Employee employee = GetEmployee(); employee.salary > 100) { ... }
8

It depends on whether you want to write clean code or not. When C was first being developed, the importance of clean code wasn't fully recognized, and compilers were very simplistic: using nested assignment like this could often result in faster code. Today, I can't think of any case where a good programmer would do it. It just makes the code less readable and more difficult to maintain.

3
  • 1
    I agree that it's less readable but only if the assignment is the only thing in the if statement. If it's something like if ((x = function()) > 0) then I think that's perfectly fine.
    – ashishduh
    Commented Mar 24, 2014 at 19:29
  • 2
    @user1199931 Not really. If you're scanning the code to get a general understanding of it, you'll see the if and continue, and miss the fact that there is a change of state. There are exceptions (for, but the keyword itself says that it is both looping and managing the loop state), but in general, a line of code should do one, and only one thing. If it controls flow, it shouldn't modify state. Commented Mar 26, 2014 at 13:41
  • 3
    "Clean code" is a very subjective term, unfortunately. Commented Jan 11, 2016 at 22:01
6

Suppose you want to check several conditions in a single if, and if any one of the conditions is true, you'd like to generate an error message. If you want to include in your error message which specific condition caused the error, you could do the following:

std::string e;
if( myMap[e = "ab"].isNotValid() ||
    myMap[e = "cd"].isNotValid() ||
    myMap[e = "ef"].isNotValid() )
{
    // Here, 'e' has the key for which the validation failed
}

So if the second condition is the one that evaluates to true, e will be equal to "cd". This is due to the short-circuit behaviour of || which is mandated by the standard (unless overloaded). See this answer for more details on short-circuiting.

2
  • Interesting, but isn't it also for showing cleverness?
    – Wolf
    Commented Apr 23, 2019 at 9:50
  • Using const char* e; instead of std::string e; would make it faster, but the similarity of the lines seems to ask for a loop over {"ab", "cd", "ef"};.
    – Wolf
    Commented Oct 1, 2021 at 10:15
2

Doing assignment in an if is a fairly common thing, though it's also common that people do it by accident.

The usual pattern is:

if (int x = expensive_function_call())
{
  // ...do things with x
}

The anti-pattern is where you're mistakenly assigning to things:

if (x = 1)
{
  // Always true
}
else
{
  // Never happens
}

You can avoid this to a degree by putting your constants or const values first, so your compiler will throw an error:

if (1 = x)
{
  // Compiler error, can't assign to 1
}

= vs. == is something you'll need to develop an eye for. I usually put whitespace around the operator so it's more obvious which operation is being performed, as longname=longername looks a lot like longname==longername at a glance, but = and == on their own are obviously different.

8
  • 10
    They're both anti patterns. And as for if ( 1 == x ), it's not the natural way of expressing the test, and any compiler worth its salt will warn for if ( x = 1 ), so there's really no reason for being ugly here either. Commented Jul 16, 2013 at 16:10
  • 1
    The 1 == x pattern, anti or not, comes from Code Complete so you'll see it fairly often. It's a bit ugly, but might be a reasonable requirement for people that have a problem getting this right, serving as training wheels.
    – tadman
    Commented Jul 16, 2013 at 16:17
  • 3
    @tadman The way to teach people is by annoying them with warnings about this typo, not to switch to yoda conditions.
    – stefan
    Commented Jul 16, 2013 at 16:18
  • 2
    @stefan Especially as most compilers have an option to turn warnings into errors. Commented Jul 16, 2013 at 16:21
  • @tadman: And reasonable compiler warnings catch more instances of this mistake than Yoda conditions do. And Yoda conditions only help when you remember to use them. Commented Jul 16, 2013 at 16:22
0

a quite common case. use

if (0 == exit_value)

instead of

if (exit_value == 0)

this kind of typo will cause compile error

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