Thread: Is this an approved use of goto?

Originally Posted by jim_0

You can just replace the code after cleanup: where you have the goto?

I could make a cleanup function I suppose. Maybe change my destroySystemSnapshot() to the code after the label... Isn't keeping the cleanup code relevant to the function inside of it nicer though?

O_o

You should employ the "weak resource pattern" in C (Example code is included because I don't think this is C++ code.) or "RAII" pattern in C++.

I didn't have any time for the code; the code is garbage, but the idea is represented.

This code is not a valid use of `goto'; the resources are simple enough to move to components.

Soma

Code:

systemSnapshot * createSnapshot()
{
    // createArray should now return null on failure
    systemSnapshot * s;
    s = ( systemSnapshot * ) createArray( NULL, 1, sizeof( systemSnapshot ), zeroArray ) ) );
    if (s) {
        s->processIds = ( DWORD * )queryAllProcessIds( &s->arraySize ) ) );
    }
    return s;
}

void destroySnapshot(systemSnapshot * f)
{
    if ( snapshot->processIds ) free( f->processIds );
    if ( snapshot->processNames )
        {
            for ( int i = 0; i < snapshot->arraySize; ++i ) free( snapshot->processNames[i] );
            free( snapshot->processNames );
        }
    if (s->processNames){
    for ( int i = 0; i < snapshot->arraySize; ++i )
            snapshot->processNames[i] = ( char * )getProcessNameFromHandle( processHandles[i], false );
    )
    }
    free( snapshot );
}

HANDLE * queryProcessHandlesFromIds(systemSnapshot * f)
{
    if (f) {
        return ( HANDLE * )queryProcessHandlesFromIds( snapshot->processIds, snapshot->arraySize )
    }
    return NULL;
}

void destroyProcessHandlesFromIds(HANDLE * f)
{
    if (f)
    {
        for ( int i = 0; i < snapshot->arraySize; ++i ) CloseHandle( processHandles[i] );
            free( processHandles );
    }
}

systemSnapshot * createSystemSnapshot()
{
    systemSnapshot * snapshot = createSnapshot(/**/);
    HANDLE * processHandles = queryProcessHandlesFromIds(/**/);

    if (snapshot && processHandles && (snapshot->processNames = ( char ** )createArray( NULL, snapshot->arraySize, sizeof( char * ), zeroArray ))) {
        for ( int i = 0; i < snapshot->arraySize; ++i )
        snapshot->processNames[i] = ( char * )getProcessNameFromHandle( processHandles[i], false );
    } else {
        destroyProcessHandlesFromIds(/**/);
        destroySnapshot(/**/);
    }
}

This is not a valid use of goto. Don't use MSDN as a source of inspiration. It's garbage and utter garbage as far as C++ is concerned. For C, it's another story.
In C++, you would use RAII to accomplish this, so you should make wrappers that releases your resources and wrap your resources into them.
Alternatively, you can use boost's scope exit macro to perform cleanup code to release resources. Not as clean as RAII, though.

Yes, with those C-style functions what you did is one one of the nicer things you can do.
But my favorite C++ solution would need more work, and could only pay off in a larger project.
That solution would be:

• Don't call those functions directly, but create own methods (or static functions), which encapsulates them and checks their return value. In case they failed, then throw an exception with a nice, friendly error message.

This can't work cleanly in every case: sometimes tricks are required, like catching the exception in the middle, freeing up some resources there and throwing it again. Actually a good C++ program should be designed in a way which makes the usage of exceptions most natural. And try to create a real class instead of the static functions, because you could store some resources in private fields, and would not need to pass these all the time.

Oh and i did not include an implementation for that exception class, although it is not hard to implement something like it.

Code:

int main() {
    Kitten *kitten = NULL;

    try {
        kitten = myClass::getKitten(123);
        
        // other calls here
        // ...
        
    } catch (Exception *ex) {
        ex->print();
    }

    // Delete classes and free up stuff what is not NULL
    delete kitten;

    return 0;
}

Kitten* myClass::getKitten(int id) {
    Kitten *kitten;
    
    kitten = OS_Function_Get_Kitten(id);
    
    if (kitten == OS_NO_KITTEN) {
        throw(new Exception("Out of kittens."));
    }
    
    if (kitten == OS_INVALID_ID) {
        throw(new Exception("The kitten id is invalid."));
    }
    
    return kitten;
}

Actually a good C++ program should be designed in a way which makes the usage of exceptions most natural.

O_o

Actually, newbies shouldn't be trying to teach nonsense by example.

1): The functionality has no reason to be a "static method".

2): Catching an exception to perform cleanup is a misuse of the exception mechanism as exists in C++. As basically everyone said, you should use "RAII" in C++. Using "RAII" properly eliminates the need for "tricks [...] like catching the exception in the middle, freeing up some resources there and throwing it again".

3):
DO NOT THROW UNPROTECTED RESOURCES!

Your `catch' block leaks memory.

Soma

IMO, in some cases, trying to avoid goto's is similar to optimizing code that doesn't need optimization. In the case of the example shown, a common error / exit clean up point, the code as shown with the goto is probably easier to follow than the more sophisticated methods.

O_o

Clean code isn't about performance; clean code is about the programmer. The use of "RAII" or the "weak resource pattern" doesn't require sophistication. Both idioms are native to the language. Both should be taught very early in the education of a new programmer. The canonical expressions of both idioms are ridiculously easy to follow. Also, "one object is one resource" is blindingly obvious from a design standpoint which makes the code easy to navigate at higher levels.

This, by the by, is the C++ forum; you should be aware by now (I've told you.) that `goto' doesn't work in C++ to provide a "common error / exit clean up point". Exceptions trump your `goto', and I'm not talking about "probably easier to follow" or "sophisticated methods" or anything else you'd like to say about `goto'. The exception mechanism in C++ doesn't honor `goto'; the "cleanup" code marked by a `goto' label isn't going to execute unless you deal with any exception.

Soma

Yup, a delete can be put there.

O_o

You are a tremendously awful C++ programmer.

No wonder you need your "trick"...

Soma