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

A

10

12

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?

Autopilot answered 15/4, 2011 at 13:15 Comment(4)

I used to think returning NULL on failure was a good idea, until recently when I worked on some inherited code where functions could return NULL for all kinds of reasons. It's not fun debugging null pointers :) – Riflery 15/4, 2011 at 13:22

Abusing macros? You've never seen real macro abuse, sonny. – Garber 15/4, 2011 at 13:26

@Garber The macros I've seen have led to a phobia of macros, hence my deep desire to avoid them! :-) – Autopilot 15/4, 2011 at 13:29

@Riflery The thing that led me in the direction of returning NULL was SDL. It seemed like a neat pattern as it means you don't need to separately test the out pointer for a NULL. – Autopilot 15/4, 2011 at 13:40

T

6

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.

Ternan answered 15/4, 2011 at 13:23 Comment(11)

can you explain a bit more deeply? – Dowling 15/4, 2011 at 13:27

Using the 'break' functionality of while loops lets you go straight to where you need to clean-up. Can whomever down voted this please explain why? – Ternan 15/4, 2011 at 13:30

I'm also interested to know why this got down-voted - to me it looks similar to the goto examples, just without the goto. – Autopilot 15/4, 2011 at 13:34

@james: oh, clever! +1 from me :) – Dowling 15/4, 2011 at 13:35

@Kragen said in another way: "similar to 10, but expressed as 1+2+3+4" ("just without the 10"). +1, I like this idea too, however I dislike that you consider it better "because it's without goto". – Vicennial 15/4, 2011 at 13:45

@Johannes Schaub: Not a fan of try... catch? – Ternan 15/4, 2011 at 13:51

What if you have multiple possible error conditions, some that share cleanup and some that don't? This will work in simple cases, but even then seems like a rather obtuse work-around for a fairly simple (and arguably clearer) goto. – Onder 15/4, 2011 at 13:52

There's probably not enough space to give an example in the comments but I'd like to see one. – Ternan 15/4, 2011 at 14:2

This solution fails if you need to do error checking inside of a loop or another while. The macros' break will break out of the loop/while but not the outer while (0). – Aneroid 15/4, 2011 at 16:10

the code snippet is not a best practice at all, there is an over engineering with the loop. All logic could be done without loop, it makes no sense, it is a place to do an error in future. I would strictly recommend to avoid such macro expressions. if you really want to reduce some constant NULL conditions checks redefine the memory allocation functions with your hooks inside to check the NULL and make some save action and exit, because program normally should not have any NULL pointer recoverable errors in termes of memory allocation. If there are some then the should be handled explicitly. – Spam 18/2, 2012 at 21:56

@ykhrustalev: Ok, you've not explained your point very well but about the macros... I personally wouldn't use them either but I put them in becuase the questioner had them. I also don't see how it can introduce errors in the future; if everything succeeds the execution will follow one path and if it fails it'll follow another. – Ternan 19/2, 2012 at 0:19

I

13

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;

Isostasy answered 15/4, 2011 at 13:31 Comment(0)

T

6

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;