Labels for premature function exit + cleanup

Just wondering how you guys feel about this sort of thing:

Code:

template<typename T> File<T>* FileSystem::Open( const char* szName, int iMode )
{
    FILE*       fRead = 0;
    File<T>*    fCreated = new File<T>;
    int         iAbsPathLen = strlen( szName ) + strlen( g_szDataRoot ) + 1;
    char*       szAbsPath = new char[iAbsPathLen];

    sprintf( szAbsPath, "%s%s", g_szDataRoot, szName );

    if ( iMode & FS_OPEN_READ )
        fRead = fopen( szAbsPath, "rb" );
    else if ( iMode & FS_OPEN_WRITE )
        fRead = fopen( szAbsPath, "wb" );
    else
    {
        Error::GetSingleton().Log( true, "FileSystem::Open: Invalid mode" );
        delete[] szAbsPath;
        delete fCreated;
        return 0;
    }

    if ( !fRead )
    {
        Error::GetSingleton().Log( true, "FileSystem::Open: Unable to open '%s'", szAbsPath );
        delete[] szAbsPath;
        delete fCreated;
        return 0;
    }

}

To me the two error situations duplicating the same cleanup code seems a bit, well, pointless. I've seen something along the lines of this being done (mainly in the Linux source code):

Code:

template<typename T> File<T>* FileSystem::Open( const char* szName, int iMode )
{
    FILE*       fRead = 0;
    File<T>*    fCreated = new File<T>;
    int         iAbsPathLen = strlen( szName ) + strlen( g_szDataRoot ) + 1;
    char*       szAbsPath = new char[iAbsPathLen];

    sprintf( szAbsPath, "%s%s", g_szDataRoot, szName );

    if ( iMode & FS_OPEN_READ )
        fRead = fopen( szAbsPath, "rb" );
    else if ( iMode & FS_OPEN_WRITE )
        fRead = fopen( szAbsPath, "wb" );
    else
    {
        Error::GetSingleton().Log( true, "FileSystem::Open: Invalid mode" );
        goto exitFail;
    }

    if ( !fRead )
    {
        Error::GetSingleton().Log( true, "FileSystem::Open: Unable to open '%s'", szAbsPath );
        goto exitFail;
    }

    // Return successfully here...

exitFail:
    delete[] szAbsPath;
    delete fCreated;
    return 0;
}

Thoughts?

Just wondering how you guys feel about this sort of thing:

Labels make me uneasy in higher level programming languages as they are rarely encountered; atleast in my experience.

I'd probably go with the first one just because.. uh.. i dunno... I wonder if the compiler would adequately optimize the code to avoid redundancy so that both your code samples would be equivalent after compilation?..?..?

Code:

1: ifnot() jmp 4
2: Error::GetSingleton().Log( true, "FileSystem::Open: Invalid mode" );
3: jmp 6;
4: if(fRead) jmp 8;
5: Error::GetSingleton().Log( true, "FileSystem::Open: Unable to open '%s'", szAbsPath );
6: delete[] szAbsPath;
7: delete fCreated;
8: return 0;

Kinda curious on others' opinions as in this case the goto label logic is clear in your code, which anyone should understand.

I'd choose the latter (the 2nd example), but only for C code, not C++ code.

If I was writing C++, I'd prefer to put new/delete allocations in a class of some sort using the RAII idiom, so that when I define a variable in a function, I don't have to worry about cleaning it up (just like if you forget to call close() for ifstream, it'll close itself once it goes out of scope).

Using RAII is probably the best way to go. You can also separate that code into another function that returns false on error, then do your error handling in one place if it does.

In this example, szAbsPath could be a string or vector<char>. Also, it appears that fCreated doesn't need to be created yet, so you could initialize it to 0 and create it later when you know it will be valid.

Yep, smart pointers, such as auto_ptr are the way to go.

Okey dokey.

For example, if you must use C-style file handling (in C++, the C++ iostreams should be preferred), you can easily use Boost.shared_ptr to manage the handle:

Code:

boost::shared_ptr<FILE> handle(fopen(filename, mode), fclose);
fread(handle.get(), ...);

The file is automatically closed when handle goes out of scope (unless a copy was made).