Thread: Is this an approved use of goto?

  1. #1
    Registered User MilleniumFalcon's Avatar
    Join Date
    Feb 2014
    Posts
    33

    Is this an approved use of goto?

    I'm building an interface for interacting with processes on Windows. In order to not have messy error handling code where I'm duplicating a lot of the same behavior for every error, I decided to use goto. I've seen this style before used in MSDN documentation and I've heard of it being used in the Linux kernel. I was wondering though if this is actually an accepted style or if people will yell at me for posting code like this. Here is a function where I use that style:

    Code:
    systemSnapshot * createSystemSnapshot( )
    {
        HANDLE * processHandles = NULL;
        systemSnapshot * snapshot = NULL;
    
    
        if ( !( snapshot = ( systemSnapshot * )createArray( NULL, 1, sizeof( systemSnapshot ), zeroArray ) ) ) return NULL;
    
    
        if ( !( snapshot->processIds = ( DWORD * )queryAllProcessIds( &snapshot->arraySize ) ) ) goto cleanup;
        if ( !( processHandles = ( HANDLE * )queryProcessHandlesFromIds( snapshot->processIds, snapshot->arraySize ) ) ) goto cleanup;
        if ( !( snapshot->processNames = ( char ** )createArray( NULL, snapshot->arraySize, sizeof( char * ), zeroArray ) ) )
            goto cleanup;
        else
            for ( int i = 0; i < snapshot->arraySize; ++i )
                snapshot->processNames[i] = ( char * )getProcessNameFromHandle( processHandles[i], false );
    
    
        for ( int i = 0; i < snapshot->arraySize; ++i ) CloseHandle( processHandles[i] );
        free( processHandles );
    
    
        return snapshot;
    
    
        cleanup:
            if ( snapshot->processIds ) free( snapshot->processIds );
            if ( processHandles )
            {
                for ( int i = 0; i < snapshot->arraySize; ++i ) CloseHandle( processHandles[i] );
                free( processHandles );
            }
            if ( snapshot->processNames )
            {
                for ( int i = 0; i < snapshot->arraySize; ++i ) free( snapshot->processNames[i] );
                free( snapshot->processNames );
            }
            free( snapshot );
    
    
            return NULL;
    }
    Yes, I'm using win32 functions along with several of my interface functions, but that's not relevant to this post.

  2. #2
    Lurking whiteflags's Avatar
    Join Date
    Apr 2006
    Location
    United States
    Posts
    9,616
    As opposed to writing a cleanup function and calling it...

  3. #3
    Registered User
    Join Date
    Nov 2013
    Posts
    107
    You can just replace the code after cleanup: where you have the goto?

  4. #4
    Registered User MilleniumFalcon's Avatar
    Join Date
    Feb 2014
    Posts
    33
    Quote Originally Posted by jim_0 View Post
    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?
    Last edited by MilleniumFalcon; 03-16-2014 at 12:37 PM.

  5. #5
    Registered User
    Join Date
    Apr 2013
    Posts
    1,658
    In the code I've written for companies, it's somewhat common to have goto's that branch to a label that contains common cleanup code for a function that deletes (or frees) any allocated resources, and closes any open handles that should be closed at function exit.

  6. #6
    Master Apprentice phantomotap's Avatar
    Join Date
    Jan 2008
    Posts
    5,108
    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(/**/);
        }
    }
    “Salem Was Wrong!” -- Pedant Necromancer
    “Four isn't random!” -- Gibbering Mouther

  7. #7
    Registered User MilleniumFalcon's Avatar
    Join Date
    Feb 2014
    Posts
    33
    Quote Originally Posted by phantomotap View Post
    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
    It's C++, but only because my version of Code::Blocks didn't happen to give me the option to compile my DLL as C. I decided just to go with it in the current project I'm writing also, which is where took my sample code from.

    I might try to implement the style you mentioned I suppose.

  8. #8
    Registered User MutantJohn's Avatar
    Join Date
    Feb 2013
    Posts
    2,665
    Hmm... Why don't you get a version of Code::Blocks that happens to give you the option to compile your code in C?

    Otherwise, this is C++ and in C++ you don't need a goto because there's try-catch blocks and exceptions.

  9. #9
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Location
    Inside my computer
    Posts
    24,654
    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.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

  10. #10
    Registered User
    Join Date
    Mar 2014
    Posts
    3
    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;
    }
    Last edited by bahamut123; 03-18-2014 at 06:45 PM.

  11. #11
    Master Apprentice phantomotap's Avatar
    Join Date
    Jan 2008
    Posts
    5,108
    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
    “Salem Was Wrong!” -- Pedant Necromancer
    “Four isn't random!” -- Gibbering Mouther

  12. #12
    Registered User
    Join Date
    Apr 2013
    Posts
    1,658
    don't need a goto ...
    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.

  13. #13
    Registered User
    Join Date
    Mar 2014
    Posts
    3
    Quote Originally Posted by phantomotap View Post
    Catching an exception to perform cleanup is a misuse of the exception mechanism as exists in C++.
    You don't get the main point. Exceptions are for giving the user the most appropriate information about the malfunction. All the other "trick" is needed because of the low level and bad design of this language.

    Quote Originally Posted by phantomotap View Post
    As basically everyone said, you should use "RAII" in C++.
    RAII doesn't exclude exceptions. It was created just to support exceptions...

    Quote Originally Posted by phantomotap View Post
    Your `catch' block leaks memory.
    Yup, a delete can be put there.

  14. #14
    Master Apprentice phantomotap's Avatar
    Join Date
    Jan 2008
    Posts
    5,108
    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
    “Salem Was Wrong!” -- Pedant Necromancer
    “Four isn't random!” -- Gibbering Mouther

  15. #15
    Master Apprentice phantomotap's Avatar
    Join Date
    Jan 2008
    Posts
    5,108
    Yup, a delete can be put there.
    O_o

    You are a tremendously awful C++ programmer.

    No wonder you need your "trick"...

    Soma
    “Salem Was Wrong!” -- Pedant Necromancer
    “Four isn't random!” -- Gibbering Mouther

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Approved Nintendo Programmer for hire
    By DScoder in forum Projects and Job Recruitment
    Replies: 0
    Last Post: 07-24-2007, 09:24 PM
  2. Why is goto so bad?
    By Munkey01 in forum C++ Programming
    Replies: 14
    Last Post: 01-02-2003, 07:11 PM
  3. Approved Slang..
    By Cheeze-It in forum A Brief History of Cprogramming.com
    Replies: 16
    Last Post: 10-06-2002, 09:58 PM