CODEWITHSUNDEEP
c++boolean
Soeren Zimmermann
Soeren Zimmermann

Reputation: 159

c++ bool concatenation with |=

I'm wondering about the way to concatenate bools.

What I sometimes need is to get a flag whether at least one object in a list of objects has a certain value/state/is focused etc..

What I do is this:

bool bHasState( false );
for ( auto o : MyObjectList )
{
   bHasState |= ( o->state() == SOME_STATE );
}

Some of my colleagues always replace this with

bHasState = bHasState || ( o->state() == SOME_STATE );

Is this just a syntactic difference, or is my way wrong, dangerous or slow?

Upvotes: 4

Views: 1494

Answers (2)

Matteo Italia
Matteo Italia

Reputation: 126927

Expanding from the comments: the main difference here is that your version always evaluates the right-hand expression, while the version with || (or the proposed if version, which is essentially the same) doesn't, as || has short-circuit evaluation.1

The performance difference boils down to balancing the cost of a branch (an easily predicted one, since it's going to be always taken until you find an element that makes bHasState true, and then always not taken) with the cost of calling state() on every item, which may be extremely cheap (if it's a straight inline accessor, so the only cost is going to be a potential cache miss) or quite costly (if state() isn't inline, performs more complex calculations or - say - has to acquire a busy mutex).2

Still, if you were to decide that branching at each iteration isn't going to be so costly, probably you should go one step further: just break out of the loop when you find the first item whose state matches what you are looking for

bool bHasState( false );
for ( auto o : MyObjectList ) {
    if(o->state() == SOME_STATE) {
        bHasState = true;
        break;
    }
}

as it's true that the || branch is going to be easily predicted, but not looping at all over irrelevant items is surely going to be faster, especially it MyObjectList contains many elements.

Incidentally, the same exact semantic can be reproduced with this horrible standard library one-liner:

bool bHasState = std::any_of(MyObjectList.begin(), MyObjectList.end(),
    [](MyObject const& o) { return o->state() == SOME_STATE; });

  1. IOW, it evaluates the right hand expression only if the left hand expression is false; somebody pointed out that, if || is overloaded, it isn't short-circuit anymore, still, here I wouldn't think this is the case - from the context we seem to be dealing with regular integers.

  2. Notice that, if the compiler can prove that calling state() has no observable side-effects (which can be trivially done if it's just an inline getter) it may transform the && to & or the opposite as it prefers, as having or not having the branch isn't technically observable (as far as the C++ standard is concerned).

Upvotes: 8

Bathsheba
Bathsheba

Reputation: 234865

The replacement is not quite the same. Do please read on:

bHasState |= ( o->state() == SOME_STATE ); is an idiomatic way of switching on a bit if the right hand side is true, or leaving it as it is if the right hand side is false. But it does require the evaluation of o->state(), which if that's expensive, the || alternative might be better.

That said, to me the way you currently have it is perfectly clear. (Personally I'd go one stage further and remove the redundant parentheses, which make the expression appear more complicated than it really is.)

But as a rule of thumb, I also wouldn't fiddle with code that works as you can introduce really subtle bugs. Currently o->state() is always evaluated, it wouldn't be if you changed it to bHasState = bHasState || ( o->state() == SOME_STATE );, bHasState was already true, and || was not overloaded. (Note that || is not short-circuited if || is overloaded.)

Upvotes: 4

Related Questions