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

It never occurred to me to use goto or do { } while(0) for error handling in this way - its pretty neat, however after thinking about it I realised that in many cases I can do the same thing by splitting the function out into two:

int Foo(void)
{
    // Initialise things that may need to be cleaned up here.
    char* somePtr = malloc(1024);
    if (somePtr = NULL)
    {
        return NULL;
    }

    if (FooInner(somePtr) < 0)
    {
        // Something went wrong so clean-up.
        free(somePtr);
        return NULL;
    }

    return somePtr;
}

int FooInner(char* somePtr)
{
    if (something(somePtr) < 0) return -1;
    if (somethingElse(somePtr) < 0) return -1;
    // etc

    // if you get here everything's ok.
    return 0;
}

This does now mean that you get an extra function, but my preference is for many short functions anyway.

After Philips advice I've also decided to avoid using control flow macros as well - its clear enough what is going on as long as you put them on one line.

At the very least Its reassuring to know that I'm not just missing something - everyone else has this problem too! :-)

Answer 2

THis must be thought on at least two levels: how your functions interact, and what you do when it breaks.

Most large C frameworks I see always return a status and "return" values by reference (this is the case of the WinAPI and of many C Mac OS APIs). You want to return a bool?

StatusCode FooBar(int a, int b, int c, bool* output);

You want to return a pointer?

StatusCode FooBar(int a, int b, int c, char** output);

Well, you get the idea.

On the calling function's side, the pattern I see the most often is to use a goto statement that points to a cleanup label:

    if (statusCode < 0) goto error;

    /* snip */
    return everythingWentWell;

error:
    cleanupResources();
    return somethingWentWrong;

Answer 3

The short answer is: let your functions return an error code that cannot possibly be a valid value - and always check the return value. For functions returning pointers, this is NULL. For functions returning a non-negative int, it's a negative value, commonly -1, and so on...

If every possible return value is also a valid value, use call-by-reference:

int my_atoi(const char *str, int *val)
{
        // convert str to int
        // store the result in *val
        // return 0 on success, -1 (or any other value except 0) otherwise
}

Checking the return value of every function might seem tedious, but that's the way errors are handled in C. Consider the function nc_dial(). All it does is checking its arguments for validity and making a network connection by calling getaddrinfo(), socket(), setsockopt(), bind()/listen() or connect(), finally freeing unused resources and updating metadata. This could be done in approximately 15 lines. However, the function has nearly 100 lines due to error checking. But that's the way it is in C. Once you get used to it, you can easily mask the error checking in your head.

Furthermore, there's nothing wrong with multiple if (Action() == 0) return -1;. To the contrary: it is usually a sign of a cautious programmer. It's good to be cautious.

And as a final comment: don't use macros for anything but defining values if you can't justify their use while someone is pointing with a gun at your head. More specifically, never use control flow statements in macros: it confuses the shit out of the poor guy who has to maintain your code 5 years after you left the company. There's nothing wrong with if (foo) return -1;. It's simple, clean and obvious to the point that you can't do any better.

Once you drop your tendency to hide control flow in macros, there's really no reason to feel like you're missing something.

Answer 4

What about this?

int NS_Expression(void)
{
    int ok = 1;
    ok = ok && NS_Term();
    ok = ok && Emit("MOVE D0, D1\n");
    ok = ok && NS_AddSub();
    return ok
}