Date: Sun, 4 Feb 2007 07:57:43 -0500 (EST)
From: "Robert P. J. Day" <rpjday@mindspring.com>
To: Theodore Tso <tytso@mit.edu>
cc: Randy Dunlap <randy.dunlap@oracle.com>,
       "Ahmed S. Darwish" <darwish.07@gmail.com>, linux-kernel@vger.kernel.org
Subject: Re: A CodingStyle suggestion
In-Reply-To: <20070204124819.GC12943@thunk.org>
Message-ID: <Pine.LNX.4.64.0702040754370.442@CPE00045a9c397f-CM001225dbafb6>
References: <20070203215848.GA10440@Ahmed> <20070203135951.564e5fe2.randy.dunlap@oracle.com>
 <20070204124819.GC12943@thunk.org>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2494
Lines: 85

On Sun, 4 Feb 2007, Theodore Tso wrote:

> On Sat, Feb 03, 2007 at 01:59:51PM -0800, Randy Dunlap wrote:
> > On Sat, 3 Feb 2007 23:58:48 +0200 Ahmed S. Darwish wrote:
> > >
> > > In CodingStyle Chapter 16 "Function return value and names", why not
> > > adding a comment about the favorable community way of checking the return
> > > value. ie:
> > >
> > > ret = do_method();
> > > if (ret) {
> > >    /* deal with error */
> > > }
> > >
> > > and not other ways like:
> > >
> > > if (do_method()) or if ((ret = do_method()) > value) ...
> > >
> >
> > I like it.  Please cc: Andrew Morton <akpm@osdl.org> on it.
> > Hopefully he will merge it.
> >
>
> I'm going to have to disagree.  Sometimes if the main flow of the code
> is down, it's actually better to do this:
>
> 	if ((err = do_foo()) < 0)
> 		return (err);
> 	if ((err = do_bar(current, filp)) < 0)
> 		return (err);
> 	if ((err = do_quux(filp, buffer)) < 0) {
> 		close(filp);
> 		return (err);
> 	}
>
> Than to do something like this:
>
> 	err = do_foo();
> 	if (err < 0)
> 		return (err);
> 	err = do_bar(current, filp);
> 	if (err < 0)
> 		return (err);
> 	err = do_quux(filp, buffer);
> 	if (err < 0) {
> 		close(filp);
> 		return (err);
> 	}
>
> The first is more concise, and it draws the reader's eye to what's
> really going on.  The cleanup/return error path is less important,
> and and it's pretty clear what's going on just from glancing at it.
>
> 						- Ted

i doubt you're going to get consensus on the *best* way to write this,
but it would be worthwhile to at least agree on the really *bad*
variations that shouldn't be used, the most vile one being:

  ret = whatever();
  if (ret) {
	...
  }

when there are no more references to "ret" inside the "if" construct,
which means there was no need to declare "ret" in the first place.

i'm guessing we can all agree on *that* but, beyond that, i think it's
going to be a matter of personal preference.

rday

-- 
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://www.fsdev.dreamhosters.com/wiki/index.php?title=Main_Page
========================================================================
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/