Is it ok to use goto in the following situation?

Question

Ok, the question of the century :)

Before you say or think anything, let me tell you that I've read couple of similar questions about this very topic, but I didn't find clear solution for my problem. My case is specific, and typical for system programmers I think.

I have this situation very often. I hate gotos, don't really know why, probably because everybody is yelling that it's bad. But until now I did not find better solution for my specific scenario, and the way I do it currently might be uglier than the use of goto.

Here is my case: I use C++ (Visual C++) for Windows application development, and quite often I use bunch of APIs in my routines. Suppose following situation:

int MyMemberFunction()
{
    // Some code... //

    if (!SomeApi())
    {
        // Cleanup code... //

        return -1;
    }

    // Some code... //

    if (!SomeOtherApi())
    {
        // Cleanup code... //

        return -2;
    }

    // Some more code... //

    if (!AnotherApi())
    {
        // Cleanup code... //

        return -3;
    }

    // More code here... //

    return 0; // Success
}

So after each Api I have to check if it succeeded, and abort my function if it did not. For this, I use whole bunch of // Cleanup code... //, often pretty much duplicated, followed by the return statement. The function performs, say, 10 tasks (e.g. uses 10 Apis), and if task #6 fails, I have to clean up the resources created by previous tasks. Note that the cleanup should be done by the function itself, so exception handling cannot be used. Also, I can't see how much-talked RAII can help me in this situation.

The only way I've thought of, is to use goto to make jump from all such failure cases to one cleanup label, placed at the end of the function.

Is there any better way of doing this? Will using goto considered bad practice in such situation? What to do then? Such situation is very typical for me (and for system programmers like me, I believe).

P.S.: Resources that need to be cleanup up, are of different types. There might be memory allocations, various system object handles that need closure, etc.

UPDATE:

I think people still did not get what I wanted (probably I'm explaining badly). I thought the pseudo-code should be enough, but here is the practical example:

1. I open two files with CreateFile. If this step fails: I have to cleanup the already-open files handle(s), if any. I will later read part of one file and write into another.

2. I use SetFilePointer to position read pointer in first file. If this step fails: I have to close handles opened by previous step.

3. I use GetFileSize to get target file size. If api fails, or file size is abnormal, I have to make cleanup: same as in previous step.

4. I allocate buffer of specified size to read from first file. If memory allocation fails, I have to close file handles again.

5. I have to use ReadFile to read from first file. If this fails, I have to: release buffer memory, and close file handles.

6. I use SetFilePointer to position write pointer in second file. If this fails, same cleanup has to be done.

7. I have to use WriteFile to write to second file. If this fails, bla-bla-bla...

Additionally, suppose I guard this function with critical section, and after I call EnterCriticalSection in the beginning of the function, I have to call LeaveCriticalSection before every return statement.

Now note that this is very simplified example. There might be more resources, and more cleanup to be done - mostly same, but sometimes a little bit different, based on which step did fail. But let's talk within this example: how can I use RAII here?

Answer 1

Use of goto is not needed, it is error prone, resulting in redundant and rather unsafe code.

Use RAII and you do not have to use goto. RAII through smart pointers is ideal for your scenario.

You ensure all your resources in the scope are RAII managed(either use smart pointers or your own resource managing classes for them), whenever a error condition occurs all you have to do is return and RAII will magically free your resources implicitly.

Answer 2

RAII will work for this as long as the resources created by previous tasks are maintained in the form of classes/objects that clean up after themselves. You mentioned memory and system object handles, so let's use those as a starting point.

// non RAII code:
int MyMemberFunction() { 
    FILE *a = fopen("Something", "r");

    if (!task1()) {
       fclose(a);
       return -1;
    }

    char *x = new char[128];

    if (!task2()) {
        delete [] x;
        fclose(a);
        return -2;
    }
}

RAII-based code:

int MyMemberFunction() { 
    std::ifstream a("Something");
    if (!task1())
        return -1; // a closed automatically when it goes out of scope

    std::vector<char> x(128);

    if (!task2())
        return -2; // a closed, x released when they go out of scope
    return 0; // again, a closed, x released when they go out of scope
}

Also note that if you normally expect things to work, you can write the code to portray that a bit more closely:

int MyMemberFunction() {
    bool one, two;

    if ((one=task1()) && (two=task2()))
        return 0;

    // If there was an error, figure out/return the correct error code.
    if (!one)
        return -1;
    return -2;
}

Edit: Although it's fairly unusual, if you really need to use c-style I/O, you can still wrap it up into a C++ class vaguely similar to an iostream:

class stream { 
    FILE *file;
public:
    stream(char const &filename) : file(fopen(filename, "r")) {}
    ~stream() { fclose(file); }
};

That's obviously simplified (a lot), but the general idea works perfectly fine. There's a less obvious, but generally superior approach: an iostream actually uses a buffer class, using underflow for reading and overflow for writing (again, simplifying, though not as much this time). It's not terribly difficult to write a buffer class that uses a FILE * to handle the reading/writing. Most of the code involved is really little more than a fairly thin translation layer to provide the right names for the functions, rearrange the parameters to the right types and orders, and so on.

In the case of memory, you have two different approaches. One is like this, writing a vector-like class of your own that acts purely as a wrapper around whatever memory management you need to use (new/delete, malloc/free, etc.)

Another approach is to observe that std::vector has an Allocator parameter, so it's really already just a wrapper, and you can designate how it obtains/frees memory. If, for example, you really needed its back-end to be malloc and free, it would be fairly easy to write an Allocator class that used them. This way most of your code would follow normal C++ conventions, using std::vector just like anything else (and still supporting RAII as above). At the same time, you get complete control over the implementation of the memory management, so you can use new/delete, malloc/free, or something directly from the OS if needed (e.g., LocalAlloc/LocalFree on Windows).

Answer 3

Use boost:

• scoped_ptr for handles, etc
• scoped_lock to control mutex.