3

A recurring pattern in a piece of inherited code is that a chain of functions is called and the chain aborted, as soon as one of the functions returns a certain value. This value shall then be used for following computation. For the sake of demonstration the breaking value is 0. Consider the following program:

#include <stdio.h>

static int n;

static int foo(int x)
{
    fprintf(stderr, "%d(%d) ", x, n++);
    return x ? 0 : n;
}

static void with_if(void)
{
    int rc;
    n = 0;

    do {
        if( (rc = foo(1)) ) break;
        if( (rc = foo(2)) ) break;
        if( (rc = foo(3)) ) break;
        if( (rc = foo(4)) ) break;
        if( (rc = foo(0)) ) break;
        if( (rc = foo(5)) ) break;
    } while(0);

    fprintf(stderr, ">>%d<<\n", rc);
}

void with_short_circuit(void)
{
    int rc;
    n = 0;

       (rc = foo(1))
    || (rc = foo(2))
    || (rc = foo(3))
    || (rc = foo(4))
    || (rc = foo(0))
    || (rc = foo(5));

    fprintf(stderr, ">>%d<<\n", rc);
}

int main(int argc, char *argv[])
{
    with_if();
    with_short_circuit();
    return 0;
}

Note that the short circuited variant is not only more concise, it's (IMHO) also far easier to read and reason about, since you don't have to push all the other surrounding statements into your mental stack, when reading that code. So I by large prefer the short circuited variant.

As far as GCC-4.9.3 and Clang-3.6.2 are concerned with_if and with_short_circuit are identical (they produce the very same assembly output).

What I'm worried about is, that the outcome of the boolean operator chain is ignored (if compiled with -Wall GCC emits a warning about it, Clang remains silent though) and that this might be seen as a chance for optimization. Of course calling foo causes a side effect, hence in my understanding of the C language standard is should be safe to use boolean short circuit for control flow like this. Bit I'm not quite sure about it.

datenwolf
  • 159,371
  • 13
  • 185
  • 298
  • What warning? AFAICS, the chained OR is well-defined. – Oliver Charlesworth Aug 22 '15 at 10:18
  • The warning is that the result of the boolean operator isn't used. you could silence this warning by assigning to a bool variable `myBool` then writing `(void)myBool;`. I don't believe that this can be optimised away by the compiler unless the functions called have no side effects. – James Snook Aug 22 '15 at 10:23
  • 1
    Assigning to `rc` is also an obvious side effect, so that must be ok. On the other hand, I don't find the second version more readable at all. I would rather consider putting the calls in another function and use early returns to terminate it. That would be more readable. :-) – Bo Persson Aug 22 '15 at 10:24
  • 2
    You're using `do`...`while` loops for something other than an actual loop. You're using it to get exactly the effect of a `goto` statement without actually writing the word `goto`. In that case, to make it easier on the next maintainer, please just write `goto` so that it is obvious what is going on. –  Aug 22 '15 at 10:34
  • I agree with 'hvd' - the 'while' loop doesn't look right. The second version is much better although I'd format it differently, but that's purely personal preference and doesn't change the meaning of the code in any way (let's start a format war!) – Skizz Aug 22 '15 at 10:55
  • @JamesSnook No need for an intermediate variable. Just cast the entire expression to `void`. – n. m. could be an AI Aug 22 '15 at 11:13
  • @hvd: Given the fact that C has so many syntactic shortcomings, one has to resort to resuse certain statements in way other than their original intention. the `do{}while(0)` construct is an accepted pattern in both macro scope isolation and for short circuiting. Also while I largely agree with using `goto`s for skipping over large spans of code, if all you want is getting out of a block, the `break` is cleaner. As for putting it into another function: Doesn't work if your inner code depends on symbols in the local scope. – datenwolf Aug 22 '15 at 11:42
  • @hvd: Also the `do{}while(...)` construct allows for conditional cleanup on block exit, by putting statements into the `while` clause, terminated with a zero-comma `… ,0`. Yes it's dirty. But if you've read is often enough (or written it) then you don't have to actively parse these constructs in your mind. You just see it and know "oh yes, that's what it does" without thinking about it. IMHO asking for code to be readable does not mean that it should be written on a first grader level; it's perfectly fine (and encourageable) to write code that's readable by being obvious in what it does. – datenwolf Aug 22 '15 at 11:47
  • @Skizz: The formatting you see there I came up with after years and years of programming practive. It takes some getting used to (it's unusual) and like many other formatting style in my code it's an aquired taste. But in whatever project I collaborated it took only a few weeks until me peers would pick it up. Yes it looks awkward at first, but once you've got used to it its much more readable *and* maintainable; due to the formatting itself you can see with a glance the overall structure of the code. Of course one can abuse the formatting to mess with peoples' minds, but who does that ;) – datenwolf Aug 22 '15 at 11:51
  • "the `do{}while(0)` construct is an accepted pattern in both macro scope isolation and for short circuiting" -- It's accepted in macro definitions for lack of a better alternative, but (arguably) you do have a better alternative here. You're the first I've seen to seriously defend it the way you're using it. Different experiences, perhaps. "Also the `do{}while(...)` construct allows for conditional cleanup on block exit, by putting statements into the `while` clause" -- Yuck. What benefit does that have over putting them at the end of the `{}` body? Or in the `goto` version, before the label? –  Aug 22 '15 at 12:50
  • If you want to disable the warning, cast the data to `void` explicitly. – Yakk - Adam Nevraumont Aug 22 '15 at 12:58
  • @Yakk: I'm aware of that hack. I'd rather avoid it and live with the warning :) – datenwolf Aug 22 '15 at 13:25
  • Please pick one of C and C++. Now I'm wondering when KLANG is finally done. – fuz Aug 22 '15 at 14:10
  • 2
    @datenwolf It really isn't a hack. It states "discard this data" explicitly. Your code base should compile without warnings, because the alternative is that your warnings are routinely ignored. Warnings that are routinely ignored can easily mask warnings that *matter*. Check each warning, confirm it isn't a problem, and arrange for it not to happen. – Yakk - Adam Nevraumont Aug 22 '15 at 14:32
  • @FUZxxl: KLANG experienced a 3 year delay due to this: http://optores.com/index.php/products/1-1310nm-mhz-fdml-laser – datenwolf Aug 22 '15 at 16:48
  • @datenwolf Now that's neat. I gladly wait for KLANG a bit longer. – fuz Aug 22 '15 at 16:49
  • @Yakk: You're right of course that code should compile without warnings. However I also think that some warnings should be left it. If you enforce a "RESOLVE ALL THE WARNINGS" policy you might end up with people throwing in typecasts or other things that should be carefully deliberated all over the place. I'm currently fixing a driver consisting of some 20k LoC and about every 10 lines or so you find something where a variable is cast `volatile` to make the compiler shut up, instead of fixing the actual problem. – datenwolf Aug 22 '15 at 16:52

4 Answers4

5

This is called a "discarded-value expression", and it has to be evaluated:

N4296, 5§11:

In some contexts, an expression only appears for its side effects. Such an expression is called a discarded-value expression. The expression is evaluated and its value is discarded.

alain
  • 11,939
  • 2
  • 31
  • 51
3

C/C++ are not allowed to optimize out function calls like this, so you are completely safe. The standard guarantees both the short-circuiting and the order of evaluation of your with_short_circuit expression, so any standard-compliant compiler will produce the code with the right behavior.

My guess as to why the compiler issues the warning is that a stand-alone expression statement without obvious side effects is usually a mistake. In your case, however, side effects are very obvious. Since you know for sure what you are doing, you can silence this specific warning with a diagnostic pragma.

Community
  • 1
  • 1
Sergey Kalinichenko
  • 714,442
  • 84
  • 1,110
  • 1,523
0

The GCC in fact does make a hard guarantee about the behaviour of any code that produces a side effect. You are quite safe, so long as you are not trying to create a delay or measure execution speed.

However, your code in the while(0) would actually be considered bad design by most. Code that performs that way is usually written like:

if( (rc = foo(1)) ) ;
    else if( (rc = foo(2)) ) ;
    else if( (rc = foo(3)) ) ;
    else if( (rc = foo(4)) ) ;
    else if( (rc = foo(0)) ) ;
    else if( (rc = foo(5)) ) ;

In addition, it is not usually considered appropriate to put an = inside a conditional expression, if for no other reason than the similarity between == and =.

warren
  • 563
  • 2
  • 13
  • 2
    "put an = inside a conditional" - I seem to remember seeing that very pattern in K&R! – Skizz Aug 22 '15 at 10:52
  • @warren: Hency the surrounding parenthesis. I usually compile my code with the warning-turned-error enabled for assignments at places where a conditional is expected. There are a few usefull scenarios for doing assignment in conditional, for example when chaining a number of function calls which may error, to conditional into an error handler using the error code. Having a own assignment-then-if block for each function invocation is tedious to write and exhausting to read (IMHO). – datenwolf Aug 22 '15 at 11:54
0

I ran a test, the result speaks for itself (figure below).

I, also, expected the compiler to honor function calls with side effects.

source code and output showing a discarded function call

Then, consulting the document below, section 5.14 (link to pdf), we find: "Unlike &, && guarantees left-to-right evaluation: the second operand is not evaluated if the first operand is false." Of course, the OR operator is implemented likewise (applying DeMorgan's law).

Working Draft, Standard for Programming Language C++, N4296

A Koscianski
  • 115
  • 1
  • 5