5

I often write code which ends up being long sequences something like

int error;

error = do_something();
if (error) {
    return error;
}

error = do_something_else(with, some, args);
if (error) {
    return error;
}


error = do_something_yet_again();
if (error) {
    return error;
}

return 0;

I'm searching for a cleaner way to write this that to some extent avoids the repeated identical checks. So far, I've written an ERROR_OR macro, which works something like

#define ERROR_OR(origerr, newerr)           \
    ({                                      \
        int __error_or_origerr = (origerr); \
        (__error_or_origerr != 0)           \
                ? __error_or_origerr        \
                : (newerr);                 \
    })

which allows the original code to become something like

int error = 0;

error = ERROR_OR(error, do_something());
error = ERROR_OR(error, do_something_else(with, some, args));
error = ERROR_OR(error, do_something_yet_again());

return error;

This is (in my opinion) a little cleaner. It's also less understandable, since the function of the ERROR_PRESERVE macro isn't apparent unless you read its documentation and/or implementation. It also doesn't solve the problem of repetition, just makes it easier to write all the (now implicit) checks on a single line.

What I'd really like to re-write this all as would be the following:

return ERROR_SHORT_CIRCUIT(
    do_something(),
    do_something_else(with, some, args),
    do_something_yet_again()
);

The hypothetical ERROR_SHORT_CIRCUIT macro would

  • Take a variable number of expressions in its argument list
  • Evaluate each expression in order
  • If every expression evaluates to zero, evaluate to zero itself
  • If any expression evaluates to nonzero, immediately terminate and evaluate to the value of that last expression

This last condition is where my short-circuit diverges from a straightforward use of the || operator -- since this will evaluate to 1 instead of the error value.

My initial attempt at writing this is the following:

#define ERROR_SHORT_CIRCUIT(firsterr, ...)          \
    ({                                              \
        int __error_ss_firsterr = (firsterr);       \
        (__error_ss_firsterr != ERROR_NONE)         \
                ? __error_ss_firsterr               \
                : ERROR_SHORT_CIRCUIT(__VA_ARGS__); \
    })

This has two obvious problems:

  • It doesn't handle its base-case (when __VA_ARGS__ is a single value)
  • C doesn't support recursive macros

I've looked into some recursive macro hacks, but I dislike using that degree of pre-processor magic -- too much room for something to be subtly wrong. I've also considered using real (possibly variadic) functions, but this would require either

  • giving up the short-circuit behavior
  • passing the functions in as pointers, and therefore normalizing their signatures

and both of these seem worse than the original, explicit code.

I'm interested to hear advice on the best way to handle this. I'm open to many different approaches, but my ultimate goal is to avoid repetition without hurting readability.

(I suppose it's obvious I'm suffering some envy of the behavior of the || operator in languages like Ruby).

Austin Glaser
  • 352
  • 1
  • 12
  • 2
    I don't think this is better, just less readable. Also the `error-or` has completely different behaviour than the code you show first. Don't get too fancy with macros; a lot of projects have more error-handling code than anything else. If your's is one of them, so be it. Concentrate on maintainable and readble code. (on a sidenoe: `({ … })` is a gcc extension. – too honest for this site Sep 07 '17 at 18:12
  • I have a feeling this question is going to be closed on account of it being primarily opinion-based. However, you could make your code more compact by putting each function call block on a single line, e.g., `if ((error = do_something())) return error; if ((error = do_something_else(with, some, args))) return error; if ((error = do_something_yet_again())) return error;` – r3mainer Sep 07 '17 at 18:12
  • 1
    Btw: `if ( error ) return error;` is one of the few cases I'd not use braces and maybe single-line it. – too honest for this site Sep 07 '17 at 18:14
  • Personally, I like using macros for boilerplate error-handling code, somewhat along the lines of your `ERROR_OR()`. The practice makes the program logic stand out much more clearly from the error handling than it does without them. I do not worry about trying to minimize the amount of error-handling code the macro expansions produce. – John Bollinger Sep 07 '17 at 18:19
  • @JohnBollinger: Problem is the first snippet does something very different than the second. And changing the code flow within a macro to the outside quickly gets you into maintenance problems.. – too honest for this site Sep 07 '17 at 19:11
  • @olaf The `ERROR_OR` snippet has the exact same behavior as the before. [The ternary operator is short-circuit](https://stackoverflow.com/questions/33417436/does-the-ternary-operator-short-circuit-in-a-defined-way), so once `err` becomes non-zero none of the functions are evaluated. – Austin Glaser Sep 11 '17 at 14:56
  • @AustinGlaser: Short circuiting does not even apply to the ternary operator. So no, it is not. – too honest for this site Sep 11 '17 at 15:01
  • @AustinGlaser: It also cannot be used with statements and code following the one before will still be executed. – too honest for this site Sep 11 '17 at 15:12
  • @Olaf I appreciate your advice, but you're wrong about the ternary's behavior. [The C99 standard](http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf) (pp. 90), states "The second operand is evaluated only if the first compares unequal to 0; the third operand is evaluated only if the first compares equal to 0." This is the entire reason I defined it as a macro rather than a function. Furthermore, I've *tested* code using this macro and have seen that it behaves correctly. – Austin Glaser Sep 12 '17 at 16:14
  • @AustinGlaser: Read my comment carefully again and try to understand all implications. I explicitly commented on your "exact same behavior as the before", which I interpreted as the call/if combination plus the `returns`. – too honest for this site Sep 12 '17 at 16:17

2 Answers2

4

I'd use code like:

if ((error = do_something()) != 0 ||
    (error = do_something_else(with, some, args)) != 0 ||
    (error = do_something_yet_again()) != 0)
    return error;
return 0;

It's fully defined because there are sequence points before each || operator. It doesn't really need a macro. It only runs into problems when you allocate resources or do other operations between function calls, but that is different from what your example code shows. At least 90% of the battle was creating the sequence of do_something_or_other() functions that make it easy to handle the error sequencing.

Jonathan Leffler
  • 730,956
  • 141
  • 904
  • 1,278
2

Another option:

int error = 0;
do {
    // Note: extra parens suppress assignment-as-conditional warning

    if ((error = do_something())) break;
    if ((error = do_something_else())) break;
    if ((error = do_yet_another_thing())) break;
    error = do_a_final_thing();
} while (0);
return error;
Lee Daniel Crocker
  • 12,927
  • 3
  • 29
  • 55
  • I'll admit, I've never been a fan of `do { ... } while (0);` as a substitute for goto, and in this case I think I prefer the explicit checking. Nevertheless, thanks for the advice! – Austin Glaser Sep 12 '17 at 16:18