Exactly. Writing libraries in C means you have to be much more strict than simple throwaway single execution binaries.
What our company has ended up doing is wrappering most low level functions with the common code around it. For example, fopen() may fail and return EINTR if the process received a signal just as it was opening a file (we use signals to tell processes to reread their config so this does happen albeit rarely) so the wrapper calls fopen() in a do/while loop that repeats on EINTR (and a few others). It saves you from writing the same 50 odd lines each time you want to open a simply open a file.
In an exception led world you'd still have to do the same thing, but with different syntactic sugar. You can't just fail and bubble up the exception without dealing with the few exceptions you must deal with and repeat, and you can't just repeat on every exception as most will just fail again and again. You end up writing code to do the same thing just in different styles.
Checking the return values of every single function call, and dealing with it, can make the code verbose (one line of code and then 10+ lines dealing with errors) but it is worth it in the end for bullet proof programs. Also code where you have to undo all of the work already done in the function up until that point, which can get a little tedious, i.e.:-
It's the job of anything calling makefoo() to deal with the various errors it can return, but the idea is to return the minimum number of unique error codes as necessary to avoid proliferation of error codes to the higher and higher functions. Many calling functions will only really care about success or failure, and will just use the return code to log out the reason for the failure.
The wrappers help deal with many situations; did fwrite() write all of the bytes we wanted or just some of them? Well, our wrapper around fwrite will handle short writes and repeat the call depending on the result of ferror().
> Also code where you have to undo all of the work already done in the function up until that point, which can get a little tedious, i.e.:-
Sorry pal, you're doing it wrong. At least, this is not the way I've done it and seen it done in large C code bases.
You're supposed to have only one return statement, and one block that frees everything. For example if you initialize `foo`, `bar`, and `qux` to `NULL`. Then testing these pointers for `NULL` de facto tells you how far you got in the function, and which buffers need to be freed. Just before your one single return statement (can't emphasize this enough) you call `free` on all of them regardless of success or failure. It's much more composeable than what you described - allocations for `foo`, `bar`, `qux` can fail, the ones that will be not yet allocated at that point in time will be `NULL`, and `free(NULL)` is a harmless no-op.
None of this business of "I've got to return now, let's see, how many of these buffers do I need to release at this point in time?", with varying amounts of the same free statement appearing redundantly. Write the cleanup block once when the pointers are about to fall out of scope, have it able to run in both success and failure cases and be done with it. Think of it as a more manual RAII if you like.
As for what to replace those early `return`s with, the two common schools are `goto` into the cleanup block, or repeatedly checking some kind of failure status variable before performing new actions.
> You can nest under if(thing!=NULL) but then you end up with indentation creep.
I am not suggesting any nesting of anything. Repeatedly checking at the same indentation level.
> You can use the goto pattern if you like but some folks will tell you that goto's are never, ever to be used.
What matters more to you, getting stuff right or repeating adages that other people have said out of context? `goto` is probably the cleanest way to do it in plain C.
> When it comes down to it the logic is basically identical and it's just down to code formatting.
No actually, it is quite a bit more than style, doing it the way alexkus has it is much much much less maintainable when it's done all over a large code base. He's got `foo`, `bar`, `qux`. What if a year later some future maintainer totally unfamiliar with the code needs to add another one? Then it's up to that person to find all of the exit paths, make sure that `foo`, `bar`, `qux` and the new thing are freed in all cases. Doing that is a lot harder if the free statements are all over the place and repeated several times instead of in a single block.
>> I am not suggesting any nesting of anything. Repeatedly checking at the same indentation level.
This could be considered wasteful. You end up checking if something is NULL, if it is you jump to the cleanup code and immediately check again. Nesting may be more elegant.
>> What matters more to you, getting stuff right or repeating adages that other people have said out of context? `goto` is probably the cleanest way to do it in plain C.
Writing code in a way consistent with the team I work with and the established codebase.
>> What if a year later some future maintainer totally unfamiliar with the code needs to add another one?
Then they need to read what the function is doing and understand it before they mess with the code, just like in any other situation.
>> Doing that is a lot harder if the free statements are all over the place and repeated several times instead of in a single block.
More verbose certainly, but if the code is written in small, discrete functional blocks then it shouldn't really impact much.
He repeats frees, you repeat tests. An indenter would repeat neither but has indent readability to consider.
Frankly, don't trust anyone that tells you that they have the one true way to do things.
It's true that there is an extra compare. I think it's a small cost for maintainable code.
> Then they need to read what the function is doing and understand it before they mess with the code, just like in any other situation.
Sounds great, however, the time they spent figuring out your haphazard, repetitive and confusing free() statements could be better spent somewhere else. When I said that my way is more "composeable" I meant that adding, removing, or re-ordering operations is a cheaper operation for the programmer. Follow this style and you'll spend less time trying to read and figure out code because it will fit the existing convention and will be bleedingly obvious where the buffers are released.
I may have been a bit hyperbolic with calling this "correct" or "supposed" however I didn't make up these conventions, I advocate them because I have seen them work really well and I have seen yours create mounds of inflexible spaghetti.
>> It's true that there is an extra compare. I think it's a small cost for maintainable code.
And I don't think it's the only way to achieve maintainable code. That's all I'm saying.
>> Sounds great, however, the time they spent figuring out your haphazard, repetitive and confusing free() statements
If it's the coding standard of the product that you have a small block of this at the top before you start actually writing the function then it doesn't take any more time for a coder to understand than any other way around, the important part is consistency.
>> I advocate them because I have seen them work really well and I have seen yours create mounds of inflexible spaghetti.
What's mine? I'm not advocating any of them, just sticking to one. I still don't think yours is any better than (for instance) -
type function()
{
type2 *thing = (cast) malloc (size);
type2 *thing2;
if (thing)
{
thing2 = (cast2) malloc (size2);
if(thing2)
{
// do some stuff here
// and some more stuff
...
free(thing2);
}
free(thing);
}
return code;
}
A pattern which auto-unrolls as it exits without the need for more tests, and to me is every bit as maintainable as the goto cleanup; pattern.
Spaghetti code (to me) is more about encapsulation and modularisation failures than it is about the content of any individual function.
What our company has ended up doing is wrappering most low level functions with the common code around it. For example, fopen() may fail and return EINTR if the process received a signal just as it was opening a file (we use signals to tell processes to reread their config so this does happen albeit rarely) so the wrapper calls fopen() in a do/while loop that repeats on EINTR (and a few others). It saves you from writing the same 50 odd lines each time you want to open a simply open a file.
In an exception led world you'd still have to do the same thing, but with different syntactic sugar. You can't just fail and bubble up the exception without dealing with the few exceptions you must deal with and repeat, and you can't just repeat on every exception as most will just fail again and again. You end up writing code to do the same thing just in different styles.
Checking the return values of every single function call, and dealing with it, can make the code verbose (one line of code and then 10+ lines dealing with errors) but it is worth it in the end for bullet proof programs. Also code where you have to undo all of the work already done in the function up until that point, which can get a little tedious, i.e.:-
It's the job of anything calling makefoo() to deal with the various errors it can return, but the idea is to return the minimum number of unique error codes as necessary to avoid proliferation of error codes to the higher and higher functions. Many calling functions will only really care about success or failure, and will just use the return code to log out the reason for the failure.The wrappers help deal with many situations; did fwrite() write all of the bytes we wanted or just some of them? Well, our wrapper around fwrite will handle short writes and repeat the call depending on the result of ferror().