Thread: 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.

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).