Getting rid of an ugly C construct

Question

I have inherited a (large) piece of code which has an error tracking mechanism where they pass in a boolean variable to all the methods they call and on errors at various stages of execution the method is stopped and returns, sometimes a default value.

Something like (BEFORE):

#include <iostream.h>
int fun1(int par1, bool& psuccess)
{
    if(par1 == 42) return 43;
    psuccess = false;
    return -1;
}
int funtoo(int a, bool& psuccess)
{
    int t = fun1(a, psuccess);
    if(!psuccess)
    {
        return -1;
    }
    return 42;
}
void funthree(int b, bool& psuccess)
{
    int h = funtoo(b, psuccess);
    if(!psuccess)
    {
         return;
    }
    cout << "Yuppi" << b;
}
int main()
{
    bool success = true;
    funthree(43, success);
    if(!success)
    {
        cout<< "Life, universe and everything have no meaning";
    }
}

Please note, that this is a mixture of C and C++ code, exactly the way the project is in.

Now, comes a piece of C magic: "someone" somewhere defined a macro:

#define SUCCES_OR_RETURN  if(!psuccess) return

And the program above becomes (AFTER):

#include<iostream.h>
int fun1(int par1, bool& psuccess)
{
    if(par1 == 42) return 43;
    psuccess = false;
    return -1;
}
int funtoo(int a, bool& psuccess)
{
    int t = fun1(a, psuccess);
    SUCCES_OR_RETURN -1;
    return 42;
}
void funthree(int b, bool& psuccess)
{
    int h = funtoo(b, psuccess);
    SUCCES_OR_RETURN ;
    std::cout << "Yuppi" << b;
}
int main()
{
    bool success = true;
    funthree(43, success);
    if(!success)
    {
        cout<< "Life, universe and everything have no meaning";
    }
}

The question: I am wondering if there is a nicer way to handle this kind of error tracking or I have to live with this. I personally don't like the abuse of the C macro SUCCES_OR_RETURN ie. that once it is called with a parameter, and in other cases it is called without, feels like a real return statement, but I did not find any better solutions to this ancient design.

Please note that due to platform restrictions we have several restrictions, but regardless of it I am willing to hear opinions about these two:

• throwing exceptions. The code is a mixture of C and C++ functions calling each other and the compiler sort of does not support throw (accepts in the syntax but does nothing with it, just a warning). This solution is sort of the standard way of solving this problem in a C++ environment.
• C++11 features, this goes to a tiny embedded platform with an obscure and ancient "almost" C++ compiler which wasn't made to support the latest C++ features. However for future reference I am curios if there is anything C++11 offers.
• template magic. The compiler has problems understanding complex templated issues, but again I am willing to see any solutions that you can come up with.

Edit

Also, as @BlueMoon suggested in the commend, creating a global variable is not working since at a very beginning of the function chain calling the success variable is a member variable of a class, and there are several objects of this class created, each of them needs to report its success status :)

Answer 1

There's a great breakdown of hybrid C and C++ error handling strategies here:

• http://blog.sduto.it/2014/05/a-c-error-handling-style-that-plays.html

To quote the linked article, your options largely boil down to:

• Return an error code from functions that can fail.
• Provide a function like Windows's GetLastError() or OpenGL's glGetError() to retrieve the most recently occurring error code.
• Provide a global (well, hopefully, thread-local) variable containing the most recent error, like POSIX's errno.
• Provide a function to return more information about an error, possibly in conjunction with one of the above approaches, like POSIX's strerror function.
• Allow the client to register a callback when an error occurs, like GLFW's glfwSetErrorCallback.
• Use an OS-specific mechanism like structured exception handling.
• Write errors out to a log file, stderr, or somewhere else.
• Just assert() or somehow else terminate the program when an error occurs.

It seems like the author of the code you have inherited picked a rather strange way, passing a pointer to a boolean [sic] for the function to work with seems rather unusual.

The article has some great examples, personally I like this style:

libfoo_widget_container_t container = NULL;
libfoo_error_details_t error = NULL;
if (libfoo_create_widgets(12, &container, &error) != libfoo_success) {
    printf("Error creating widgets: %s\n", libfoo_error_details_c_str(error));
    libfoo_error_details_free(error);
    abort(); // goodbye, cruel world!
}

Here you get a bit of everything, passed in pointer to error type, a comparison against a success constant (rather than 0|1, a painful dichotomy between C and the rest of the world!).

I don't think it would be too much of a push to say that your macro could rather better be implemented with a goto, in any case, if a function is calling SUCCES_OR_RETURN more than once, it might be a clue that the function is doing too much. Complex cleanup, or return might be a code smell, you can read more here http://eli.thegreenplace.net/2009/04/27/using-goto-for-error-handling-in-c/

Answer 2

I have seen this style of error handling before. I call it error-oblivious manual pseudo-exceptions.

The code flow is mostly error-oblivious: you can call 3 functions in a row with the same error flag, then look at the error flag to see if any errors have occurred.

The error flag acts as a pseudo-exception, where once set we start "skipping" over normal code flow, but this is done manually instead of automatically.

If you do something and do not care if an error occurs, you can just drop the error produced and proceed on.

The ICU library handles errors in a similar way.

A more C++1y way to do this while minimizing structural differences would be to modify code to return an expected object.

An expected<T, Err> is expected to be a T, and if something went wrong it is instead an Err type. This can be implemented as a hybrid of boost::variant and C++1y's std::optional. If you go and overload most arithmetic operations on expected< T, Err > + U to return expected< decltype( std::declval<T&>() + std::declval<U>(), Err > and did some careful auto, you could allow at least arithmetic expressions to keep their structure. You'd then check for the error after the fact.

On the other hand, if the error return values are predictable based on their type, you could create a type that when cast to a given type produced an error value. Modify functions returning void to return an error object of some kind while you are at it. And now every function can

if (berror) return error_flag_value{};

which at least gets rid of that strange ; or -1; issue.