3

I have code like this, and I find it a bit hard to read:

// code1
if( (expensiveOperation1() && otherOperation() && foo()) 
     || (expensiveOperation2() && bar() && baz()) {
  // do something
}

I just changed it to the following, to make it more readable:

// code2
const bool expr1 = expensiveOperation1() && otherOperation() && foo();
const bool expr2 = expensiveOperation2() && bar() && baz();
if(expr1 || expr2){
   // one of the conditions met
}

But should I now be concerned about efficiency?

I mean, in code1, if the first conjunctive clause is fulfilled then it won't even bother to look at the second one because it's already clear that the statement will be true.

But in my more readable example, both cond1 and cond2 have to be computed. Or will the compiler be smart enough to change my code2 into code1 if expr2 is not used anywhere else?

Frank
  • 64,140
  • 93
  • 237
  • 324

7 Answers7

24

I would say it shouldn't, since they're not logically equivalent if any of the functions have side-effects.

The following would be equivalent however, and it'd have the advantage allowing you to give descriptive names to the test functions, making the code more self-documenting:

// code3
inline bool combinedOp1()
{
    return expensiveOperation1() && otherOperation() && foo();
}

inline bool combinedOp2()
{
    return expensiveOperation2() && bar() && baz();
}

And then call it as follows:

if (combinedOp1() || combinedOp2())
{
    // do something
}
John Carter
  • 53,924
  • 26
  • 111
  • 144
  • Due to lazy evaluation you'd better not have any side-effects. – tvanfosson Mar 01 '09 at 21:05
  • Agreed, I'd be surprised if it optimized this away. Of course there *shouldn't* be any side effects, but the compiler won't know that. It has to assume the worst. If the definition of your expensiveOperation is visible, it *might* optmize it, but it's not a safe bet. I like this solution though. – jalf Mar 01 '09 at 21:31
  • Hmm, you're saying that this will, in some cases, execute combinedOp2(), even if combinedOp1() is already true? Can anyone confirm that that is definitely the case? Because if that's so, then this shouldn't be the accepted answer. – Frank Mar 01 '09 at 21:35
  • 1
    No, that's not what he means. combinedOp2() is guaranteed never to be called if combinedOp1() is true. – John Carter Mar 01 '09 at 21:43
20

Maybe, but why not just make your second check incorporate the first?

// code3
bool expr = expensiveOperation1() && otherOperation() && foo();
expr = expr || (expensiveOperation2() && bar() && baz());
if(expr){
   // one of the conditions met
}

Better yet, turn things around so the least expensive check occurs first in each list, taking advantage of the lazy evaluation to skip the expensive operations entirely.

tvanfosson
  • 524,688
  • 99
  • 697
  • 795
  • Nice - one of those simple things that probably wouldn't have come to mind for me in a million years. – Michael Burr Mar 01 '09 at 21:03
  • Nice, except that expr is declared const so the second line won't compile :-) I'm guessing you meant "const bool expr1 = ...; const bool expr2 = expr1 || (...)" – SCFrench Mar 01 '09 at 21:16
  • Cut/paste error. I'd probably not make it const and reuse it. – tvanfosson Mar 01 '09 at 21:27
  • 1
    A drawback is that it harder to find descriptive names. In dehmann's version, expr1 and expr2 variables can be named to express the subconditions. – Tomas Aug 23 '10 at 12:30
  • @Tomas -- Given that it's local to the snippet, it's likely that naming the separate conditions isn't really important. The final result (e.g., `processThisAccount`) is more important than which condition triggers it (e.g., `balancePositive || overdraftEligible`). Of course, if the conditions or their results are used later (which seems unlikely since he wants to avoid the second's evaluation if possible), evaluating them separately could still be done -- though you'd likely want to introduce a method if you wanted to delay evaluation of the second until actually needed. – tvanfosson Aug 23 '10 at 13:34
  • 1
    @tvanfosson - The reason dehmann gives for "code2" is to make code1 more readable. This is without doubt best achieved with clear, distinctive names for expr1, and expr2 and COULD be a drawback of your solution. It doesn't have to be, so no downvotes from me, just wanted to point it out. – Tomas Aug 23 '10 at 14:02
  • @Tomas - by "readable" I'm assuming he means not split across different lines or on a single long line. In the original it was a single go/no-go condition and only got separated into two for "readability" -- this tells me that it (1) it's not likely that the second condition will be reused and (2) that re-using the variable name is actually preferrable since it represents a single condition. Splitting it into methods, as in the accepted answer, is a reasonable choice to which I'd probably refactor if I reused the conditions elsewhere. – tvanfosson Aug 23 '10 at 14:12
4

Well, the compiler in general will not reorder &&'s and ||'s on the off chance that the conditions have side effects. a few very smart compilers might be able to statically verify their independence, but this is going to be rare.

If possible, reorder your conditions for the cheap operations to come first, so it can short circuit the expensive ones.

SingleNegationElimination
  • 151,563
  • 33
  • 264
  • 304
2

The top answers here are answering the question with "should not" and "maybe"! That isn't a definitive answer come on!

If you want to know if your compiler is optimizing this tiny bit of code, compile your code with the "show assembly output" flag. On GCC that flag is "-S". Then look at the output assembly and it will show you EXACTLY 100% what is being compiled or not.

Then you can compare your first code snipped to the code snippet from "therefromhere" and rapidly try numerous code changes until you find one that the compiler optimizes the best ( i.e. least cycles ).

It sounds complex and scary to look at the asm output but in reality it only takes about 5 minutes to do. I have done an example here: What is the fastest way to swap values in C?

Community
  • 1
  • 1
Trevor Boyd Smith
  • 18,164
  • 32
  • 127
  • 177
  • -1 That's a great way to produce non-portable code that breaks with the next compiler update. Always try to understand what the language guarantees, only if the behavior appears to break one of those guarantees should you start analyzing compiler implementation details. – Ben Voigt Dec 21 '09 at 22:13
  • 1
    When did I ever suggest "oh and while you are looking at the compiler's output asm... make sure to make your C code non-portable". You must be trying really hard to misinterpret my writing into "make your code non-portable". Only a fool would purposely make their C code non-portable. A greater fool would non-purposely make their C code non-portable without thinking about it. – Trevor Boyd Smith Dec 23 '09 at 16:12
1

The answer to this question depends on the compiler of course. The definitive way to check is to look at the assembly generated by the compiler for this function. Most (all?) compilers have a way to do this, for instance gcc has the -S option. If for some bizarre reason yours doesn't most debuggers can show you the disassembly for a function, or there are other tools to do this with.

Logan Capaldo
  • 39,555
  • 5
  • 63
  • 78
0

The compiler can optimize if it knows that the functions in cond2 (expensiveOperation2(), bar() and baz()) are pure (ie have no side-effects). If they are pure, the simplest way to make sure the compiler knows it is to make them inline functions.

It is possible that the compiler can tell even if you don't, but its very unlikely since expensiveOperation2() probably does quite a lot of work.

FWIW, if those functions are pure, you should probably re-order them so that bar() and baz() run before expensiveOperation2() (and same for the ordering in cond1).

Paul Biggar
  • 27,579
  • 21
  • 99
  • 152
0

Good answers.

I would only add that I don't like compilers to be so aggressive in optimizing as to re-order my code.

I just want a compiler to do what it's told.

If it's able to outsmart me, it can also outsmart itself.

Mike Dunlavey
  • 40,059
  • 14
  • 91
  • 135