6

I'm learning c++ and I've run into some behaviour that I can't explain. The two pieces of code below provide different results, while I would expect them to be equivalent:

success = true;
vector<instruction>::const_iterator i;
for (i = instructions.begin(); i != instructions.end(); ++i) {
    bool up = update(*i);
    success = success && up;        
}

and

success = true;
vector<instruction>::const_iterator i;
for (i = instructions.begin(); i != instructions.end(); ++i) {
    success = success && update(*i);        
}

I have the impression that the second version always takes the initial value of the iterator. Could someone explain me the reason?

splinter123
  • 1,183
  • 1
  • 14
  • 34
  • 8
    Short circuiting – Paolo M Dec 16 '15 at 08:15
  • We do not know what `update` returns, so `success` may or may not be `true`. – PaulMcKenzie Dec 16 '15 at 08:17
  • Also note you are pre-incrementing `++i` in the first example and post-incrementing `i++` in the second. It shouldn't make a difference, but just be aware. See this [question](http://stackoverflow.com/questions/484462/difference-between-i-and-i-in-a-loop) for more detail. –  Dec 16 '15 at 08:35

2 Answers2

13

The two pieces of code are not equivalent.

The first one always calls update, while the second one will not. The reason is that && does something called short-circuit boolean evaluation. If success is false, the second half of the expression is not evaluated.

Note that you did not post what update is or what is returned. Therefore we can only assume what may be able to be returned from update, which is either going to be true or false.

If you wanted update to always be called, it should be placed first in the && expression.

PaulMcKenzie
  • 34,698
  • 4
  • 24
  • 45
  • May be worth pointing out that, while `success` is always `true` on the first iteration, it may not be after that, depending on the result of the first call to `update(*i);` – BoBTFish Dec 16 '15 at 08:18
  • 2
    Perhaps also worth commenting that a fix is to write `success = update(*i) && success;` .. and the whole thing becomes easier if you write `success=true; for (const auto& inst : instructions) success = update(inst) && success;` (On multiple lines of course) – Martin Bonner supports Monica Dec 16 '15 at 08:24
  • thanks that was the reason indeed! Strange thing is that I knew that, but I had neglected that my program actually requires that `update` be called all the time. So for the purpose of knowing the value of `success` the two formulations are equivalent, but still the outcomes of the program are very different. Thanks! – splinter123 Dec 16 '15 at 08:27
4

The operator && will not evaluate the right hand side if the result is computable with just the left hand side value. We call this short-circuiting.

Once success is false, update(*i) will no longer be called.

Aside from your first block of code (which always calls update(*i)), one fix is to use the operator & instead which always evaluates both arguments. Another fix is to write

success = update(*i) && success;

But that's vulnerable to recalcitrant refactoring. Better still, use

success &= update(*i);

But be aware of this comment by @Angew:

Using &= in a boolean context is dangerous. What if update now (or after a future refactoring) returns an int with the semantics "non-zero for true" (cf. isdigit and friends)? Remember that bool(true & 2) is false.

Bathsheba
  • 231,907
  • 34
  • 361
  • 483
  • @Angew: You raise an important point and I would agree with you in C, but in C++ we have the luxury of a "proper" Boolean type, and relational operators returning true and false, as opposed to 1 and 0. – Bathsheba Dec 16 '15 at 08:31
  • 1
    Given that computing wisdom only emanates from bug-fixing, and so therefore you're wiser than me in this respect, I've copied your caution to the answer. – Bathsheba Dec 16 '15 at 08:36