Error handling in C



  • What do you think is worse:

    int example1() {
        void *a = malloc(1000);
        void *b = malloc(2000);
        void *c = malloc(3000);
        
         if (error_condition_1) {
            free(a);
            free(b);
            free(c);
            return 1;
        }
        ...
         if (error_condition_2) {
            free(a);
            free(b);
            free(c);
            return 1;
        }
        ...
         if (error_condition_3) {
            free(a);
            free(b);
            free(c);
            return 1;
        }
         ...
            free(a);
            free(b);
            free(c);
            return 0;
    }
    

    or

    int example2() {
        int return_value = 0;
        void *a = malloc(1000);
        void *b = malloc(2000);
        void *c = malloc(3000);
        
         if (error_condition_1)
            goto error_happened;
        ...
         if (error_condition_2)
            goto error_happened;
        ...
         if (error_condition_3)
            goto error_happened;
    
            ...
        
         cleanup:
         
            free(a);
            free(b);
            free(c);
            return return_value;
            
    error_happened:
    
        return_value = 1;
        goto cleanup;
    }
    

    or

    int example3() {
        int return_value = 1;
        void *a = malloc(1000);
        void *b = malloc(2000);
        void *c = malloc(3000);
        
         if (!error_condition_1) {
            ...
            if (!error_condition_2) {
                ...
                if (!error_condition_3) {
                    ...
                    return_value = 0;
                 }
             }
         }
        free(a);
        free(b);
        free(c);
        return return_value;
    }


  • @wharrgarbl said in Error handling in C:

    What do you think is worse:

    CSI zoom & enhance:
    0_1496318929349_44bc5ad8-2aea-43b2-a9ca-4750c76d449e-image.png

    Found the worse!



  • Don't forget:

     int example4() {
         int status = E_OK;
         int return_value = 1;
         void *a = malloc(1000);
         void *b = malloc(2000);
         void *c = malloc(3000);
         
          if (status == E_OK && error_condition_1)
              status = E_FILE_NOT_FOUND;
          
          if (status == E_OK && error_condition_2)
              status = E_FILE_FOUND;
                
          if (status == E_OK && error_condition_3)
              status = E_LOGIC_FAIL;
    
          if (status == E_OK)
              return_value = 0;
    
         free(a);
         free(b);
         free(c);
         return return_value;
     }


  • Given the alternatives, the middle one is the worst. Then the first, then the last. Of course, the problem lies in using C in the first place.



  • My order:
    WORST < example1 < example2 (agh goto!) < example3-or-4 < example4-or-3 < example2 (minus the goto hysteria) < LEAST WORST



  • @CreatedToDislikeThis
    :wtf:

    :pendant:

    @CreatedToDislikeThis said in Error handling in C:

    My order:
    WORST < example1 < example2 (agh goto!) < example3 = example4 < example2 (minus the goto hysteria) < LEAST WORST

    FTFY.


  • Discourse touched me in a no-no place

    @Kian IME, example3() looks fine with a few things, but grows to become a truly horrible mess as the number of steps increases. It really helps one's code's understandability to keep nesting shallow.

    And that applies to many other languages too.



  • @CreatedToDislikeThis That may be a good idea if you're irrationally scared of goto, but gotophobia is a Java developers thing.



  • @wharrgarbl IMO the third option is the best and the first option is extremely dangerous. I've seen the middle one plenty of times in good-quality codebases.


  • ♿ (Parody)

    My biggest problem with #2 is being triggered by the missing braces. #1 just makes me irrationally angry due to all of the repetition. I might even consider something like:

    #DEFINE CLEANUP_EXAMPLE2 free(a); free(b); free(c);
    

    The nested ifs aren't terrible so long as there aren't too many of them, like @dkf said, but it has the advantage of a single cleanup and return point without even a hint of any spaghetti.



  • @wharrgarbl The problem I have with the goto-example is that the code flow has to go back. If there is an error, you jump to the end, and then jump back to the normal exit path. I mean, this works, but it doesn't feel right when reading the code.

    Also, the other problem with goto's is that they are a little-used feature (generally speaking) and much-maligned one, so it is likely that another dev reading that code will immediately get suspicious. Anything that doesn't make the code easy to read is, for me, a bad thing.

    (in other words, write code that is easy to read for a human, easy to modify and understand. Performance or other computer-related criterion is secundary, i.e. only make it less readable for performance sake when you've actually proven that this is an issue)

    Another consideration is the coding style of the rest of the project. One function that uses goto in an entire codebase is probably a bad idea (unless for very specific functions that have a good reason for it). If that's the normal coding style, well, go for it.

    I tend to avoid example 3 (nested if's) as this can easily make the code stretch quite far visually (OK, we've got big screens now, but still). And, back to the readability, I find that it makes understanding the flow slightly more difficult ("error_condition_1 was false, so we continue to the next line, but what happens if it was true... [scroll scroll scroll] oh, ok, nothing actually. Good, so let's scroll back... where was I? Here? There? Ah, right, now error_condition_2 is also false, but what happens if it wasn't..." and so on).

    So even though it is more error-prone when editing the code (i.e. if adding void* d or something else), I would prefer example 1, where reading the flow is clear (error_condition_1: ok, we abort immediately; now error_condition_2... etc.). Yes, it is longer, but longer for me is less an issue if it's easy to follow. And you're asking for the "less worse" one, so obviously you know that none of these good...



  • @remi But example 1 can easily result in the cleanup code getting out of sync, could it not? I generally place higher emphasis on reducing code duplication.



  • @boomzilla said in Error handling in C:

    The nested ifs aren't terrible so long as there aren't too many of them

    This is a simple example, in real code the nesting frequently gets out of hand.

    And to divide the code I end with functions that receive a fuckton of parameters. Last time I created some one-use structs make the things more readable.



  • @remi said in Error handling in C:

    Also, the other problem with goto's is that they are a little-used feature (generally speaking) and much-maligned one, so it is likely that another dev reading that code will immediately get suspicious.

    No C developer I know personally would blink at the sight of a goto


  • Discourse touched me in a no-no place

    @wharrgarbl said in Error handling in C:

    And to divide the code I end with functions that receive a fuckton of parameters. Last time I created some one-use structs make the things more readable.

    Sometimes that's a very good approach. One function that allocates and deallocates the resources, and an inner function (eligible for inlining) that does the core operation and is handed in the resources to use in a state ready to go.



  • @wharrgarbl said in Error handling in C:

    @remi said in Error handling in C:

    Also, the other problem with goto's is that they are a little-used feature (generally speaking) and much-maligned one, so it is likely that another dev reading that code will immediately get suspicious.

    No C developer I know personally would blink at the sight of a goto

    Well then, if they are OK with it, this clearly makes the code short and relatively easy to read, so that sounds like the best solution here. My only concern is with the flow that goes back, which means some potential for error when modifying it later ("I want to add something at the end of the function so that should be here... oops, this is the error handling code!"), but again, if this is a common pattern in your codebase, that shouldn't really be a problem.



  • I've worked with code like that before - the goto version is actually rather nice. Once you get over gotoitis. We restricted gotos to just error handling, so there wasn't the spaghetti effect people worry over.



  • @remi The current codebase here doesn't have many of that, because their original creators believed dynamic allocated memory is bad. That prevented memory leaks, but is prone to buffer overruns with all that fixed size strings.

    But example2 could be changed to:

    int example5() {
        int return_value = 0;
        void *a = malloc(1000);
        void *b = malloc(2000);
        void *c = malloc(3000);
        
         if (error_condition_1) {
            return_value = 1;
            goto cleanup;
        }
        ...
         if (error_condition_2) {
            return_value = 1;
            goto cleanup;
        }
        ...
        
        cleanup:
         
        free(a);
        free(b);
        free(c);
        return return_value;
    }


  • @LB_ Yes, a lot of code duplication means a lot of places where errors can happen. Clearly, none of the solutions is perfect (and someone already gave the perfect solution, i.e. "don't use C", so I can't make that joke again!).

    I personally favour keeping the code easy to read, and for me a bit of duplication is less an impediment to readability than, say, deeply-nested if's. Now, that may very well be my own personal style of code, and there may be different answers for different cases (if there are only 3 conditions and the clean-up implies free()'ing 20 variables, the nested-if approach is probably much easier to read!).

    I had a phase in my programming life where I valued no-duplication above almost everything else, and that lead to making a lot of silly 2-lines functions that are called in completly different locations and other such things that made code almost impossible to read (such as "this block and that one do the same thing except for one parameter so I'll make a common function... oh, and this one as well if I add one more parameter... and that one..." and I'll end up with some weird function that does nothing clear except massaging a lot of parameters). I'm now leaning more in the readability-first camp, but of course this is not a definitive rule.



  • @remi said in Error handling in C:

    I had a phase in my programming life where I valued no-duplication above almost everything else, and that lead to making a lot of silly 2-lines functions that are called in completly different locations and other such things that made code almost impossible to read (such as "this block and that one do the same thing except for one parameter so I'll make a common function... oh, and this one as well if I add one more parameter... and that one..." and I'll end up with some weird function that does nothing clear except massaging a lot of parameters). I'm now leaning more in the readability-first camp, but of course this is not a definitive rule.

    That's why you don't aim to deduplicate code - you aim to deduplicate logic.



  • @wharrgarbl Yes, that would work as well. My main point was that you shouldn't be using a feature unless you are reasonably confident that this will not trip up the other devs that may stumble upon it (and that includes you 6 months from now...). Yes, that means not using all the features of a language (or not a lot). But that's better than someone coming along later and loosing a lot of time trying to understand the code (or worse, breaking it because they thought they understood it but didn't).



  • @izzion said in Error handling in C:

    @wharrgarbl said in Error handling in C:

    What do you think is worse:

    CSI zoom & enhance:
    0_1496318929349_44bc5ad8-2aea-43b2-a9ca-4750c76d449e-image.png

    Found the worse!

    Just write your programs without errors you idiots.


  • 🚽 Regular

    int example1() {
        void *a = malloc(1000);
        void *b = malloc(2000);
        void *c = malloc(3000);
         
        int return_value = 0;
            
        do
        {
            if (error_condition_1) {
                return_value = 1;
                break;
            }
            ...
            if (error_condition_2) {
                return_value = 1;
                break;
            }
            ...
            if (error_condition_3) {
                return_value = 1;
                break;
            }
            ...
        } while(0); // For idiots allergic to goto
        
        free(a);
        free(b);
        free(c);
        return return_value;
    }
    
    int example2() {
        char aa[1000]; // These don't leave this function =P
        char bb[2000];
        char cc[3000];
    
        void *a = &aa;
        void *b = &bb;
        void *c = &cc;
        
        if (error_condition_1) {
            return 1;
        }
        ...
        if (error_condition_2) {
            return 1;
        }
        ...
        if (error_condition_3) {
            return 1;
        }
        ...
        return 0;
    }
    

    Before I get pendanted: in example2 you need to make sure no outside covfeferences to aa, bb and cc are kept.


  • Discourse touched me in a no-no place

    @remi said in Error handling in C:

    that includes you 6 months from now...

    The problem with you+6months is that he knows where you live. ;)


  • ♿ (Parody)

    @Zecc said in Error handling in C:

    Before I get pendanted: in example2 you need to make sure no outside covfeferences to aa, bb and cc are kept.

    That's already baked into the assumptions here because they're all freed when they live on the heap.



  • @Zecc example2 is what happens a lot in this codebase, and we got some buffer overflows because of that.

    And stack memory is very limited on some devices, so we have some WTFs like:

    char big_global_temporary_buffer[10000]; 
    
    int wtf_example() {
        char *a = &big_global_temporary_buffer[0];
        char *b = &big_global_temporary_buffer[1000];
        char *c = &big_global_temporary_buffer[3000];
        ...

  • 🚽 Regular

    @wharrgarbl I'm a bit out of my depth here, but if you're doing that kind of tight, low-level memory management wouldn't it somewhat more reasonable to have something like:

    union {
        struct {
            char a[1000];
            char b[2000];
            char c[7000];
        } wtf_example_1,
    
        struct {
            char d[5000];
            char e[5000];
        } wtf_example_2,
    
        char the_whole_thing[10000]
    } our_precious_memory;
    
    int wtf_example_1() {
        // Don't call wtf_example_2 while running this function,
        // neither directly or indirectly
        char *a = &our_precious_memory.wtf_example_1.a;
        char *b = &our_precious_memory.wtf_example_1.b;
        char *c = &our_precious_memory.wtf_example_1.c;
        ...
    }
    
    int wtf_example_2() {
        // Don't call wtf_example_1 while running this function,
        // neither directly nor indirectly
        char *d = &our_precious_memory.wtf_example_2.d;
        char *e = &our_precious_memory.wtf_example_2.e
        ...
    }
    
    

    At least it should make memory management a bit more centralized.



  • @Zecc I don't think the author of wtf_example was aiming for reasonable



  • @wharrgarbl said in Error handling in C:

    And stack memory is very limited on some devices, so we have some WTFs like:

    As long as they don't use globals as a way of passing arguments to function calls... :-/



  • The goto version's the best, if you could refactor it so it doesn't jump upwards. That's weird and confusing.

    Not using C at all would be ideal.



  • @cvi said in Error handling in C:

    As long as they don't use globals as a way of passing arguments to function calls...

    What about a dictionary-ish structure passed around that has all the same problems as globals or globals that exist only to save typing (declaring int foo in every function would be work)


  • ♿ (Parody)

    @wharrgarbl said in Error handling in C:

    What about a dictionary-ish structure passed around that has all the same problems as globals or globals that exist only to save typing (declaring int foo in every function would be work)

    Enlightenment thread is :arrows:.



  • @wharrgarbl Example1 requires manually keeping multiple sections of code in sync, which is annoying. Example3 gets annoying to read with more error conditions.

    Example2 would be better (mainly because it mimics exceptions) it it was written as

    int example2() {
        int return_value = 0;
        void *a = malloc(1000);
        void *b = malloc(2000);
        void *c = malloc(3000);
        
         if (error_condition_1)
            goto error_happened;
        ...
         if (error_condition_2)
            goto error_happened;
        ...
         if (error_condition_3)
            goto error_happened;
    
        ...
    
        /* no error happened if we're here; skip error handling */
        goto cleanup;
    
    error_happened:
        return_value = 1;
        
    cleanup:
        free(a);
        free(b);
        free(c);
        return return_value;
    }


  • For trolling blindly anti-goto people:

    int example2b()
    {
    	int return_value = 1; /* assume error occurs */
    	void *a = malloc(1000);
    	void *b = malloc(2000);
    	void *c = malloc(3000);
    
        	do
    	{
    		if (error_condition_1)
    			break;
    		...
    
    		if (error_condition_2)
    			break;
    		...
    
    		if (error_condition_3)
    			break;
    
    		...
    
    		/* no error happened if we're here... */
    		return_value = 0;
    	} while( false );
    
    	free(a);
    	free(b);
    	free(c);
    	return return_value;
    }
    


  • @wharrgarbl said in Error handling in C:

    @CreatedToDislikeThis That may be a good idea if you're irrationally scared of goto, but gotophobia is a Java developers thing.

    So I guess that it is usually a decent pattern?


  • :belt_onion:

    @wharrgarbl said in Error handling in C:

    @CreatedToDislikeThis That may be a good idea if you're irrationally scared of goto, but gotophobia is a Java developers thing.

    Well, to accomplish 2, you're gonna need to make return_value global, or at least accessible wherever you put your handling code. (Or your handling code is past the return and jumps up, which is sketchy for different raisins as Blakey already pointed out). Which is super sketchy. Honestly (as a Java and C# dev), that's the scariest part about that option. I think error handling is one of the very few places GOTO makes sense, but you've got to be very careful about how exactly you use it. You could also avoid that entirely by making return_value default to 1 and only changing that when it successfully finishes. Of course that's slightly different too.
    1 is bad because it's copypasta code, forget one spot and you forgot to free that new variable you added (or potentially worse side-effects, if you're doing more than free)
    3 is probably the best from a "not screw yourself over" standpoint, but it can get insanely bad style -wise

    Of course, I think exceptions are also an order of magnitude better at making your error handling code not cause more errors, so CLOSED_HERE'S_A_NICKEL_GET_A_BETTER_LANGUAGE_KID


  • Discourse touched me in a no-no place

    @cvi said in Error handling in C:

    For trolling blindly anti-goto people:

    The anti-goto fanatics tend to also be hyper-keen on not using break, continue or more than one return.


  • Discourse touched me in a no-no place

    @sloosecannon said in Error handling in C:

    exceptions are also an order of magnitude better at making your error handling code not cause more errors

    For most languages I agree. For C++… well, their exceptions make my teeth itch for some reason…


  • Winner of the 2016 Presidential Election

    @dkf said in Error handling in C:

    The anti-goto fanatics tend to also be hyper-keen on not using break, continue or more than one return.

    I remember being taught that more than one return is evil in CS 101. :facepalm: I wish I could go back in time and tell that professor how stupid his advice is. In any language that has RAII or a using construct, there's absolutely no fucking reason to avoid early returns, and even for languages without those features, the advice is questionable at best. In fact, early returns make code way more readable.


  • Considered Harmful

    @CreatedToDislikeThis said in Error handling in C:

    That's why you don't aim to deduplicate code - you aim to deduplicate logic.

    Then the cleanup macro mentioned would do the job. However, the #1 reason to use C in the first place is because you need your code to be fast and/or small. The stacked-if version would be equivalent to the goto one in that respect, only that it gets ugly after a few indentation levels. However, if your code is less than a page so you can keep an eye inside which pair of curlies you are ATM, using goto isn't that much less readable either. The first version is much less efficient though; even a good optimizer can't be expected¹ to clean that up.

    ¹ I'm ready to be proven wrong once again on this though ;)


  • Winner of the 2016 Presidential Election

    @LaoC said in Error handling in C:

    The first version is much less efficient though;

    Wait, what? Why should it be less efficient? The only possible difference is the size of the compiled program.

    even a good optimizer can't be expected¹ to clean that up.

    I don't know if any compilers actually do this (because what's the point?), but it shouldn't be too hard to recognize identical basic blocks up to a certain number of instructions.



  • @asdf said in Error handling in C:

    I don't know if any compilers actually do this (because what's the point?), but it shouldn't be too hard to recognize identical basic blocks up to a certain number of instructions.

    Both GCC and Clang do this. ;-) See compiler explorer.



  • @wharrgarbl said in Error handling in C:

    original creators believed dynamic allocated memory is bad

    Not a WTF on an embedded system with strict real time performance



  • @Helix said in Error handling in C:

    Not a WTF on an embedded system with strict real time performance

    You guys always find a edge case where the WTF would make sense. But no, our application is network bound. The only reason it needs C is it's the only thing available on some platforms.


  • kills Dumbledore

    @wharrgarbl said in Error handling in C:

    it's the only thing available on some platforms.

    Embedded platforms with no file system?



  • @Jaloopa there is a weird filesystem that doesn't support directories


  • kills Dumbledore

    @wharrgarbl DOS 1.0?



  • @Jaloopa no, but MS-DOS is one of the target platforms


  • Discourse touched me in a no-no place

    @Helix said in Error handling in C:

    Not a WTF on an embedded system with strict real time performance

    I'm writing code for a system that looks a lot like an embedded system with real time performance requirements (it's actually a supercomputer made out of a lot of those building blocks). We use dynamic memory allocation. ;)


  • :belt_onion:

    @dkf said in Error handling in C:

    @sloosecannon said in Error handling in C:

    exceptions are also an order of magnitude better at making your error handling code not cause more errors

    For most languages I agree. For C++… well, their exceptions make my teeth itch for some reason…

    Never had to use them.

    ...

    From what I hear, that's probably a good thing...


Log in to reply