Re: How to avoid the woord 'goto' (was Re: any chance of 2.6.0-test*?)

[Rob Wilkens]

Yeah, but..

My changes won't actually be incorporated (i hope) into the kernel. 
I've since learned that a goto will optimize to be just as quick as what
I wrote (by studying the optimized assembler output at other
suggestion).  Therefore, it was silly and pointless for me to spend the
10-12 minutes I did changing the code and testing it and e-mailing back
the changes.

So, there's nothing to worry about.  Goto's are here forever.

-Rob

On Mon, 2003-01-13 at 06:08, Bernd Petrovitsch wrote:
> Rob Wilkens <[e-mail]> wrote:
> [...]
> >Here's the patch if you want to apply it (i have only compile tested it,
> >I haven't booted with it).. This patch applied to the 2.5.56 kernel.
> >
> >--- open.c.orig	2003-01-12 16:17:01.000000000 -0500
> >+++ open.c	2003-01-12 16:22:32.000000000 -0500
> >@@ -100,44 +100,58 @@
> > 
> > 	error = -EINVAL;
> > 	if (length < 0)	/* sorry, but loff_t says... */
> >-		goto out;
> >+		return error;
> > 
> > 	error = user_path_walk(path, &nd);
> > 	if (error)
> >-		goto out;
> >+		return error;
> > 	inode = nd.dentry->d_inode;
> [ snipped the rest ]
> 
> You just copied the logic to "cleanup and leave" the function several 
> times. The (current, next and subsequent) maintainers at the next 
> change in that function simply _have_ to check all cases as if they 
> are different.
> Yes, _now_ you (and all others) know that they are identical. But in 6 
> month after tons of patches?
> 
> Perhaps you want to avoid goto's with:
> ----  snip (yes, it is purposely not a `diff -urN`)  ----
> switch(0==0) {
> default:
>     error = -EINVAL;
>     if (length < 0) /* sorry, but loff_t says... */
>         break;
>     error = user_path_walk(path, &nd);
>     if (error)
>         break;
>     inode = nd.dentry->d_inode;
> 
>     switch(0==0) {
>     default:
>         /* For directories it's -EISDIR, for other non-regulars - -EINVAL */
>         error = -EISDIR;
>         if (S_ISDIR(inode->i_mode))
> 	    break;
>         error = -EINVAL;
>         if (!S_ISREG(inode->i_mode))
>             break;
> 
>         error = permission(inode,MAY_WRITE);
>         if (error)
>             break;
> 
>         error = -EROFS;
>         if(IS_RDONLY(inode))
>             break;
> 
>         /* the rest omitted - the pattern should be clear */
> 
>         put_write_access(inode);
>         break;
>     }
>     path_release(&nd);
>     break;
> }
> return error;
> ----  snip  ----
> 
> FYI, backward goto's can be rewritten with:
> ----  snip  ----
> 
>     for(;;) {
>         <do something>
>         if(i_want_to_go_back)
>             continue;
>         <do something_else>
>         break;
>     }
> ----  snip  ----
> 
> Are these two more understandable because the avoid the 'goto'?
> And try to see it not from the viewpoint of a first time reader, but
> from the viewpoint of a/the maintainer/developer (who reads this code
> probably quite often)?
> 
> 	Bernd