Transitioning from C `goto` error handling paradigm to C++ exception handling paradigm

Question

I'm a C programmer learning C++. In C, there is a common goto idiom used to handle errors and exit cleanly from a function. I've read that exception handling via try-catch blocks is preferred in object-oriented programs, but I'm having trouble implementing this paradigm in C++.

Take for example the following function in C which uses the goto error handling paradigm:

unsigned foobar(void){
    FILE *fp = fopen("blah.txt", "r");
    if(!fp){
        goto exit_fopen;
    }

    /* the blackbox function performs various
     * operations on, and otherwise modifies,
     * the state of external data structures */
    if(blackbox()){
        goto exit_blackbox;
    }

    const size_t NUM_DATUM = 42;
    unsigned long *data = malloc(NUM_DATUM*sizeof(*data));
    if(!data){
        goto exit_data;
    }

    for(size_t i = 0; i < NUM_DATUM; i++){
        char buffer[256] = "";
        if(!fgets(buffer, sizeof(buffer), fp)){
            goto exit_read;
        }

        data[i] = strtoul(buffer, NULL, 0);
    }

    for(size_t i = 0; i < NUM_DATUM/2; i++){
        printf("%lu\n", data[i] + data[i + NUM_DATUM/2]);
    }

    free(data)
    /* the undo_blackbox function reverts the
     * changes made by the blackbox function */
    undo_blackbox();
    fclose(fp);

    return 0;

exit_read:
    free(data);
exit_data:
    undo_blackbox();
exit_blackbox:
    fclose(fp);
exit_fopen:
    return 1;
}

I tried to recreate the function in C++ using the exception handling paradigm as such:

unsigned foobar(){
    ifstream fp ("blah.txt");
    if(!fp.is_open()){
        return 1;
    }

    try{
        // the blackbox function performs various
        // operations on, and otherwise modifies,
        // the state of external data structures
        blackbox();
    }catch(...){
        fp.close();
        return 1;
    }

    const size_t NUM_DATUM = 42;
    unsigned long *data;
    try{
        data = new unsigned long [NUM_DATUM];
    }catch(...){
        // the undo_blackbox function reverts the
        // changes made by the blackbox function
        undo_blackbox();
        fp.close();
        return 1;
    }

    for(size_t i = 0; i < NUM_DATUM; i++){
        string buffer;
        if(!getline(fp, buffer)){
            delete[] data;
            undo_blackbox();
            fp.close();
            return 1;
        }

        stringstream(buffer) >> data[i];
    }

    for(size_t i = 0; i < NUM_DATUM/2; i++){
        cout << data[i] + data[i + NUM_DATUM/2] << endl;
    }

    delete[] data;
    undo_blackbox();
    fp.close();

    return 0;
}

I feel my C++ version didn't properly implement the exception handling paradigm; in fact, the C++ version seems even less readable and more error prone due to a build of cleanup code accumulating in the catch blocks as the function grows.

I've read that all this cleanup code in the catch blocks may be unnecessary in C++ due to something called RAII, but I'm unfamiliar with the concept. Is my implementation proper, or is there a better way to handle errors and cleanly exit a function in C++?

Answer 1

The principle of RAII is that you use a class type to manage any resource which needs cleaning up after use; that cleanup is done by the destructor.

That means you can create a local RAII manager, which will automatically clean up whatever it's managing when it goes out of scope, whether that's due to normal program flow or an exception. There should never be any need for a catch block just to clean up; only when you need to handle or report the exception.

In your case, you have three resources:

• The file fp. ifstream is already an RAII type, so just remove the redundant calls to fp.close() and all is good.
• The allocated memory data. Use a local array if it's a small fixed size (as this is), or std::vector if it needs to be dynamically allocated; then get rid of the delete.
• The state set up by blackbox.

You can write your own RAII wrapper for the "black box" malarkey:

struct blackbox_guard {
    // Set up the state on construction
    blackbox_guard()  {blackbox();}

    // Restore the state on destruction
    ~blackbox_guard() {undo_blackbox();}

    // Prevent copying per the Rule of Three
    blackbox_guard(blackbox_guard const &) = delete;
    void operator=(blackbox_guard) = delete;
};

Now you can remove all your error-handling code; I'd indicate failure through exceptions (either thrown, or allowed to propagate) rather than a magic return value, giving:

void foobar(){
    ifstream fp ("blah.txt"); // No need to check now, the first read will fail if not open
    blackbox_guard bb;

    const size_t NUM_DATUM = 42;
    unsigned long data[NUM_DATUM];   // or vector<unsigned long> data(NUM_DATUM);

    for(size_t i = 0; i < NUM_DATUM; i++){
        string buffer;

        // You could avoid this check by setting the file to throw on error
        // fp.exceptions(ios::badbit); or something like that before the loop
        if(!getline(fp, buffer)){
             throw std::runtime_error("Failed to read"); // or whatever
        }

        stringstream(buffer) >> data[i]; // or data[i] = stoul(buffer);
    }

    for(size_t i = 0; i < NUM_DATUM/2; i++){
        cout << data[i] + data[i + NUM_DATUM/2] << endl;
    }
}

Answer 2

Yes, you should use RAII (Resource Acquisition Is Initialisation) wherever possible. It leads to code which is both easy to read and safe.

The core idea is that you acquire resources during the initialisation of an object, and set up the object so that it correctly releases the resources on its destruction. The vital point why this works is that destructors run normally when scope is exited via an exception.

In your case, there is already RAII available and you're just not using it. std::ifstream (I assume that's what your ifstream refers to) indeed closes on destruction. So all the close() calls in catch can safely be omitted and will happen automatically—precisely what RAII is for.

For data, you should be using an RAII wrapper too. There are two available: std::unique_ptr<unsigned long[]>, and std::vector<unsigned long>. Both take care of memory deallocation in their respective destructors.

Finally, for blackbox(), you can create a trivial RAII wrapper yourself:

struct BlackBoxer
{
  BlackBoxer()
  {
    blackbox();
  }

  ~BlackBoxer()
  {
    undo_blackbox();
  }
};

When rewritten with these, your code would become much simpler:

unsigned foobar() {
  ifstream fp ("blah.txt");
  if(!fp.is_open()){
    return 1;
  }

  try {
    BlackBoxer b;

    const size_t NUM_DATUM = 42;
    std::vector<unsigned long> data(NUM_DATUM);
    for(size_t i = 0; i < NUM_DATUM; i++){
      string buffer;
      if(!getline(fp, buffer)){
        return 1;
      }

      stringstream(buffer) >> data[i];
    }

    for(size_t i = 0; i < NUM_DATUM/2; i++){
      cout << data[i] + data[i + NUM_DATUM/2] << endl;
    }

    return 0;
  } catch (...) {
    return 1;
  }
}

Additionally, notice that your function uses a return value to indicate success or failure. This may be what you want (if failure is "normal" for this function), or might just represent only going half-way (if failure is supposed to be exceptional too).

If it's the latter, simply change the function to void, get rid of the try–catch construct, and throw a suitable exception instead of return 1;.

Finally, even if you decide to keep the return value approach (which is perfectly valid), consider changing the function to return bool, with true meaning success. It's more idiomatic.

Answer 3

Let me rewrite that for you using c++ idiom with explanations inline to the code

// void return type, we may no guarantees about exceptions
// this function may throw
void foobar(){
   // the blackbox function performs various
   // operations on, and otherwise modifies,
   // the state of external data structures
   blackbox();

   // scope exit will cleanup blackbox no matter what happens
   // a scope exit like this one should always be used
   // immediately after the resource that it is guarding is
   // taken.
   // but if you find yourself using this in multiple places
   // wrapping blackbox in a dedicated wrapper is a good idea
   BOOST_SCOPE_EXIT[]{
       undo_blackbox();
   }BOOST_SCOPE_EXIT_END


   const size_t NUM_DATUM = 42;
   // using a vector the data will always be freed
   std::vector<unsigned long> data;
   // prevent multiple allocations by reserving what we expect to use
   data.reserve(NUM_DATUM);
   unsigned long d;
   size_t count = 0;
   // never declare things before you're just about to use them
   // doing so means paying no cost for construction and
   // destruction if something above fails
   ifstream fp ("blah.txt");
   // no need for a stringstream we can check to see if the
   // file open succeeded and if the operation succeeded
   // by just getting the truthy answer from the input operation
   while(fp >> d && count < NUM_DATUM)
   {
       // places the item at the back of the vector directly
       // this may also expand the vector but we have already
       // reserved the space so that shouldn't happen
       data.emplace_back(d);
       ++count;
   }

   for(size_t i = 0; i < NUM_DATUM/2; i++){
       cout << data[i] + data[i + NUM_DATUM/2] << endl;
   }
}

The most powerful feature of c++ is not classes, it is the destructor. The destructor allows for resources or responsibilities to be discharged or released when the scope is exited. This means you don't have to re-write cleanup code multiple times. Moreover because only constructed objects can be destructed; if you never get to an item and thus never construct it, you do not pay any penalty in destruction if something happens.

If you find yourself repeating cleanup code, that should be a flag that the code in question is not taking advantages of the power of the destructor and RAII.

Answer 4

In C, there is a common goto idiom used to handle errors and exit cleaning from a function. I've read that exception handling via try-catch blocks is preferred in object-oriented programs,

That's not true at all for C++.

But C++ has deterministic destructors instead of finally blocks (which are used, for example, in Java), and that's a game changer for error-handling code.

I've read that all this cleanup code in the catch blocks may be unnecessary in C++ due to something called RAII,

Yes, in C++ you use "RAII". Which is a poor name for a great concept. The name is poor because it puts the emphasis on initialisation (Resource Acquisition Is Initialisation). The important thing about RAII, in contrast, lies in destruction. As the destructor of a local object will be executed at the end of a block, no matter what happens, be it early returns or even exceptions, it's the perfect place for code that releases resources.

but I'm unfamiliar with the concept.

Well, for the very beginning, you can start with Wikipedia's definition:

http://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization

Or you go straight to Bjarne Stroustrup's website:

http://www.stroustrup.com/bs_faq2.html#finally

I'm sure we'd be more than happy to answer questions about particular aspects of the idiom or problems that you encounter using it :)

Is my implementation proper, or is there a better way to handle errors and cleanly exit a function in C++?

Your implementation is not what one would expect from good C++ code.

Here is an example using RAII. It uses exceptions to report errors, and destructors to perform cleanup operations.

#include <fstream>
#include <stdexcept>
#include <vector>

// C or low-level functions to be wrapped:
int blackbox();
void undo_blackbox();

// just to be able to compile this example:
FILE *fp;

// The only self-made RAII class we need for this example
struct Blackbox {
    Blackbox() {
        if (!blackbox()) {
            throw std::runtime_error("blackbox failed");
        }
    }

    // Destructor performs cleanup:
    ~Blackbox() {
        undo_blackbox();
    }   
};

void foobar(void){
    // std::ifstream is an implementation of the RAII idiom,
    // because its destructor closes the file:
    std::ifstream is("blah.txt");
    if (!is) {
        throw std::runtime_error("could not open blah.txt");
    }

    Blackbox local_blackbox;

    // std::vector itself is an implementation of the RAII idiom,
    // because its destructor frees any allocated data:
    std::vector<unsigned long> data(42);

    for(size_t i = 0; i < data.size(); i++){
        char buffer[256] = "";
        if(!fgets(buffer, sizeof(buffer), fp)){
            throw std::runtime_error("fgets error");
        }

        data[i] = strtoul(buffer, NULL, 0);
    }

    for(size_t i = 0; i < (data.size()/2); i++){
        printf("%lu\n", data[i] + data[i + (data.size()/2)]);
    }

    // nothing to do here - the destructors do all the work!
}

By the way, +1 for trying to learn a new concept in a new language. It's not easy to change your mindset in a different language! :)