Re: [PATCH 00/11] Atari Ethernet/USB patch series - for upstream and debian-kernel

["Christian T. Steigies" <cts@debian.org>]

Wouter,
On Sat, Mar 30, 2013 at 08:23:45AM +0100, Wouter Verhelst wrote:
> On 29-03-13 23:47, Christian T. Steigies wrote:
> > On Fri, Mar 29, 2013 at 09:38:31PM +0000, Thorsten Glaser wrote:
> >>
> >> Avoiding gotos for the purpose of avoiding gotos is absolutely
> >> silly (as the compiler translates them to jumps, which are the
> >> same in asm) and positively harmful (for the legibility of the
> >> code; I wish I had goto in mksh???).
> > 
> > From my Commodore Basic days I still know gotos and spaghetti. I know it
> > will be translated to jumps, but I thought we are programming in C and not
> > asm, because the readability is better. But maybe I just read too much about
> > python recently ;-)
> 
> Goto in BASIC is not the same thing as goto in C. In BASIC, a goto
> statement can go all over the place. You see a goto, you don't know
> where you'll end up. Yes, that is spaghetti code.
> 
> In C, a goto can only jump within the same function. It's possible to
> overuse it, but error unwinding is one of the few cases where it's
> sensible and good to use it. Consider:
> 
> int handle = initialize_handle();
> if(connect_handle(handle)!=CONNECTION_OK) {
> 	unintialize_handle(handle);
> 	return ECONNFAILED;
> }
> if(send_greeting(handle)!=GREETING_OK) {
> 	uninitialize_handle(handle);
> 	return EGREETFAILED;
> }
> return HANDLE_OK;
> 
> This code works, and it's fairly clear what it does; but the problem
> here is that you replicate deinitialization code. If that
> deinitialization code ever needs to change, you have to change it in
> multiple places. You might miss one, causing you a leak of sorts. It's
> of course possible to nest the ifs, but that's even worse:
> 
> int handle = initialize_handle();
> int retval = HANDLE_OK;
> if(connect_handle(handle) != CONNECTION_OK) {
> 	retval = ECONNFAILED;
> } else {
> 	if(send_greeting(handle) != GREETING_OK) {
> 		retval = EGREETFAILED;
> 	}
> }
> uninitialize_handle(handle);
> return retval;
> 
> it's easy to see that this kind of code will become unreadable once
> you're past three or four of these checks. However, the compiled code
> will be *exactly* the same as the following:
> 
> int handle = initialize_handle();
> int retval = HANDLE_OK;
> if(connect_handle(handle) != CONNECTION_OK) {
> 	retval = ECONNFAILED;
> 	goto err_out;
> }
> if(send_greeting(handle) != GREETING_OK) {
> 	retval = EGREETFAILED;
> 	goto err_out;
> }
> err_out:
> uninitialize_handle(handle);
> return retval;
> 
> This is again as readable as the first example, but the deinitialization
> is grouped at the bottom of the function.

Thanks for the detailed example, do you want to teach me to be a kernel
hacker? :-)

I don't know too much C, but I have the feeling that in your last example
uninitialize_handle is called in any case, or are commands after the goto
lable skipped if you do not jump to them? If that is the case, I do not find
this more readable. I think the last lines of your example have to look like
this:

goto no_err;
err_out:
uninitialize_handle(handle);
no_err:
return retval;

And that in my exes is spaghetti... but I understand the rest of your
reasoning.

bon appetit!
Christian