Is there a better way to do C style error handling?

Question

I'm trying to learn C by writing a simple parser / compiler. So far its been a very enlightening experience, however coming from a strong background in C# I'm having some problems adjusting - in particular to the lack of exceptions.

Now I've read Cleaner, more elegant, and harder to recognize and I agree with every word in that article; In my C# code I avoid throwing exceptions whenever possible, however now that I'm faced with a world where I can't throw exceptions my error handling is completely swamping the otherwise clean and easy-to-read logic of my code.

At the moment I'm writing code which needs to fail fast if there is a problem, and it also potentially deeply nested - I've settled on a error handling pattern whereby "Get" functions return NULL on an error, and other functions return -1 on failure. In both cases the function that fails calls NS_SetError() and so all the calling function needs to do is to clean up and immediately return on a failure.

My issue is that the number of if (Action() < 0) return -1; statements that I have is doing my head in - it's very repetitive and completely obscures the underlying logic. I've ended up creating myself a simple macro to try and improve the situation, for example:

#define NOT_ERROR(X)    if ((X) < 0) return -1

int NS_Expression(void)
{
    NOT_ERROR(NS_Term());
    NOT_ERROR(Emit("MOVE D0, D1\n"));

    if (strcmp(current->str, "+") == 0)
    {
        NOT_ERROR(NS_Add());
    }
    else if (strcmp(current->str, "-") == 0)
    {
        NOT_ERROR(NS_Subtract());
    }
    else
    {
        NS_SetError("Expected: operator");
        return -1;
    }
    return 0;
}

Each of the functions NS_Term, NS_Add and NS_Subtract do a NS_SetError() and return -1 in the case of an error - its better, but it still feels like I'm abusing macros and doesn't allow for any cleanup (some functions, in particular Get functions that return a pointer, are more complex and require clean-up code to be run).

Overall it just feels like I'm missing something - despite the fact that error handling in this way is supposedly easier to recognize, In many of my functions I'm really struggling to identify whether or not errors are being handled correctly:

• Some functions return NULL on an error
• Some functions return < 0 on an error
• Some functions never produce an error
• My functions do a NS_SetError(), but many other functions don't.

Is there a better way that I can structure my functions, or does everyone else also have this problem?

Also is having Get functions (that return a pointer to an object) return NULL on an error a good idea, or is it just confusing my error handling?

Answer 1

Besides goto, standard C has another construct to handle exceptional flow control setjmp/longjmp. It has the advantage that you can break out of multiply nested control statements more easily than with break as was proposed by someone, and in addition to what goto provides has a status indication that can encode the reason for what went wrong.

Another issue is just the syntax of your construct. It is not a good idea to use a control statement that can inadvertibly be added to. In your case

if (bla) NOT_ERROR(X);
else printf("wow!\n");

would go fundamentally wrong. I'd use something like

#define NOT_ERROR(X)          \
  if ((X) >= 0) { (void)0; }  \
  else return -1

instead.

Answer 2

It's a bigger problem when you have to repeat the same finalizing code before each return from an error. In such cases it is widely accepted to use goto:

int func ()
{
  if (a() < 0) {
    goto failure_a;
  }

  if (b() < 0) {
    goto failure_b;
  }

  if (c() < 0) {
    goto failure_c;
  }

  return SUCCESS;

  failure_c:
  undo_b();

  failure_b:
  undo_a();

  failure_a:
  return FAILURE;
}

You can even create your own macros around this to save you some typing, something like this (I haven't tested this though):

#define CALL(funcname, ...) \
  if (funcname(__VA_ARGS__) < 0) { \ 
    goto failure_ ## funcname; \
  }

Overall, it is a much cleaner and less redundant approach than the trivial handling:

int func ()
{
  if (a() < 0) {
    return FAILURE;
  }

  if (b() < 0) {
    undo_a();
    return FAILURE;
  }

  if (c() < 0) {
    undo_b();
    undo_a();
    return FAILURE;
  }

  return SUCCESS;
}

As an additional hint, I often use chaining to reduce the number of if's in my code:

if (a() < 0 || b() < 0 || c() < 0) {
  return FAILURE;
}

Since || is a short-circuit operator, the above would substitute three separate if's. Consider using chaining in a return statement as well:

return (a() < 0 || b() < 0 || c() < 0) ? FAILURE : SUCCESS;

Answer 3

One technique for cleanup is to use an while loop that will never actually iterate. It gives you goto without using goto.

#define NOT_ERROR(x) if ((x) < 0) break;
#define NOT_NULL(x) if ((x) == NULL) break;

// Initialise things that may need to be cleaned up here.
char* somePtr = NULL;

do
{
    NOT_NULL(somePtr = malloc(1024));
    NOT_ERROR(something(somePtr));
    NOT_ERROR(somethingElse(somePtr));
    // etc

    // if you get here everything's ok.
    return somePtr;
}
while (0);

// Something went wrong so clean-up.
free(somePtr);
return NULL;

You lose a level of indentation though.

Edit: I'd like to add that I've nothing against goto, it's just that for the use-case of the questioner he doesn't really need it. There are cases where using goto beats the pants off any other method, but this isn't one of them.