A CodingStyle suggestion

A CodingStyle suggestion

[Ahmed S. Darwish]

Hi all,

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) ...

A patch is ready if the replies are positive.

Thanks,
-- 
Ahmed S. Darwish
http://darwish-07.blogspot.com

Re: A CodingStyle suggestion

[Randy Dunlap]

On Sat, 3 Feb 2007 23:58:48 +0200 Ahmed S. Darwish wrote:

> Hi all,
> 
> 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) ...
> 
> A patch is ready if the replies are positive.

I like it.  Please cc: Andrew Morton <akpm@osdl.org> on it.
Hopefully he will merge it.

---
~Randy

Re: A CodingStyle suggestion

[Richard Knutsson]

Ahmed S. Darwish wrote:
> Hi all,
>
> 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 
So:

if (is_true()) {
	/* do something */
}

is alright then? If so, I agree, but please make it real clear in the 
document ;)

Richard Knutsson

Re: A CodingStyle suggestion

[Ahmed S. Darwish]

On Sat, Feb 03, 2007 at 11:56:16PM +0100, Richard Knutsson wrote:
> Ahmed S. Darwish wrote:
> >Hi all,
> >
> >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 
> So:
> 
> if (is_true()) {
> 	/* do something */
> }
> 
> is alright then? If so, I agree, but please make it real clear in the 
> document ;)

Good catch :). A small grep of `access_ok' reveals that it's always used in the 
form of:
if (!access_ok()) { .. }

I can conclude that verbal/imperative methods like `kmalloc, add_work' be 
checked as:
ret = do_work();
if (ret) { ... }
and predicate methods like `acess_ok, pci_dev_present' be checked like:
if (!access_ok) { ... }
if (pci_dev_present) { ...}

Any comments ?

-- 
Ahmed S. Darwish
http://darwish-07.blogspot.com

Re: A CodingStyle suggestion

[Roland Dreier]

 > Good catch :). A small grep of `access_ok' reveals that it's always used in the 
 > form of:
 > if (!access_ok()) { .. }
 > 
 > I can conclude that verbal/imperative methods like `kmalloc, add_work' be 
 > checked as:
 > ret = do_work();
 > if (ret) { ... }
 > and predicate methods like `acess_ok, pci_dev_present' be checked like:
 > if (!access_ok) { ... }
 > if (pci_dev_present) { ...}
 > 
 > Any comments ?

I don't think that's really the distinction that matters.  I think
really the issue is that assignment within an if is hard to read, so

	ret = foo(a, b);
        if (ret) { ... }

is clearly preferred to

	if ((ret = foo(a,b))) { ... }

However, in my opinion something like

	if (foo(a,b)) { ... }

if perfectly fine if the return value of foo is not needed anywhere
else.  In other words, there's no sense introducing a temporary
variable to hold the return value if you're never going to do anything
with it other than check it on the next line.

 - R.

Re: A CodingStyle suggestion

[Tim Schmielau]

On Sun, 4 Feb 2007, Ahmed S. Darwish wrote:
> On Sat, Feb 03, 2007 at 11:56:16PM +0100, Richard Knutsson wrote:
> > So:
> > 
> > if (is_true()) {
> > 	/* do something */
> > }
> > 
> > is alright then? If so, I agree, but please make it real clear in the 
> > document ;)
> 
> Good catch :). A small grep of `access_ok' reveals that it's always used in the 
> form of:
> if (!access_ok()) { .. }
> 
> I can conclude that verbal/imperative methods like `kmalloc, add_work' be 
> checked as:
> ret = do_work();
> if (ret) { ... }
> and predicate methods like `acess_ok, pci_dev_present' be checked like:
> if (!access_ok) { ... }
> if (pci_dev_present) { ...}

Maybe say that any functions with a side effect should be called on a line 
by themselves?

Tim

Re: A CodingStyle suggestion

[Richard Knutsson]

Ahmed S. Darwish wrote:
> On Sat, Feb 03, 2007 at 11:56:16PM +0100, Richard Knutsson wrote:
>   
>> Ahmed S. Darwish wrote:
>>     
>>> Hi all,
>>>
>>> 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 
>>>       
>> So:
>>
>> if (is_true()) {
>> 	/* do something */
>> }
>>
>> is alright then? If so, I agree, but please make it real clear in the 
>> document ;)
>>     
>
> Good catch :). A small grep of `access_ok' reveals that it's always used in the 
> form of:
> if (!access_ok()) { .. }
>
> I can conclude that verbal/imperative methods like `kmalloc, add_work' be 
> checked as:
> ret = do_work();
> if (ret) { ... }
> and predicate methods like `acess_ok, pci_dev_present' be checked like:
> if (!access_ok) { ... }
> if (pci_dev_present) { ...}
>
> Any comments ?
>   
Not really, I agree on this :)

Richard Knutsson

Re: A CodingStyle suggestion

[Randy Dunlap]

On Sat, 03 Feb 2007 16:21:18 -0800 Roland Dreier wrote:

>  > Good catch :). A small grep of `access_ok' reveals that it's always used in the 
>  > form of:
>  > if (!access_ok()) { .. }
>  > 
>  > I can conclude that verbal/imperative methods like `kmalloc, add_work' be 
>  > checked as:
>  > ret = do_work();
>  > if (ret) { ... }
>  > and predicate methods like `acess_ok, pci_dev_present' be checked like:
>  > if (!access_ok) { ... }
>  > if (pci_dev_present) { ...}
>  > 
>  > Any comments ?
> 
> I don't think that's really the distinction that matters.  I think
> really the issue is that assignment within an if is hard to read, so
> 
> 	ret = foo(a, b);
>         if (ret) { ... }
> 
> is clearly preferred to
> 
> 	if ((ret = foo(a,b))) { ... }
> 
> However, in my opinion something like
> 
> 	if (foo(a,b)) { ... }
> 
> if perfectly fine if the return value of foo is not needed anywhere
> else.  In other words, there's no sense introducing a temporary
> variable to hold the return value if you're never going to do anything
> with it other than check it on the next line.

I agree with Roland's comments here.

And with Tim's about side effects.

---
~Randy

Re: A CodingStyle suggestion

[Willy Tarreau]

On Sat, Feb 03, 2007 at 04:40:42PM -0800, Randy Dunlap wrote:
> On Sat, 03 Feb 2007 16:21:18 -0800 Roland Dreier wrote:
> 
> >  > Good catch :). A small grep of `access_ok' reveals that it's always used in the 
> >  > form of:
> >  > if (!access_ok()) { .. }
> >  > 
> >  > I can conclude that verbal/imperative methods like `kmalloc, add_work' be 
> >  > checked as:
> >  > ret = do_work();
> >  > if (ret) { ... }
> >  > and predicate methods like `acess_ok, pci_dev_present' be checked like:
> >  > if (!access_ok) { ... }
> >  > if (pci_dev_present) { ...}
> >  > 
> >  > Any comments ?
> > 
> > I don't think that's really the distinction that matters.  I think
> > really the issue is that assignment within an if is hard to read, so
> > 
> > 	ret = foo(a, b);
> >         if (ret) { ... }
> > 
> > is clearly preferred to
> > 
> > 	if ((ret = foo(a,b))) { ... }
> > 
> > However, in my opinion something like
> > 
> > 	if (foo(a,b)) { ... }
> > 
> > if perfectly fine if the return value of foo is not needed anywhere
> > else.  In other words, there's no sense introducing a temporary
> > variable to hold the return value if you're never going to do anything
> > with it other than check it on the next line.
> 
> I agree with Roland's comments here.
> 
> And with Tim's about side effects.

Generally, a function which only returns a boolean is fine in a condition,
because that's exactly how the result will be used. The problem with
functions in if() is that many ifs are used to check for errors, and
during debugging phases, it's quite common to comment out an if block.

So the functions with side effects, including those who modify the
arguments, should never be used in a if() or while() which might be
commented out.

So the following block should be OK :

	if (!netif_running(dev)) {
		reset_driver(dev);
		retry--;
	}

While the following one is dangerous :

	if (!read_next_word(hw, &word))
		return -EINVAL;

During debugging, a block like above might end up like this :

	if (!read_next_word(hw, &word));
	//	return -EINVAL;

You can imagine what will result from this code later : the
return will be uncommented and the semi-colon forgotten :

	if (!read_next_word(hw, &word));
		return -EINVAL;

We have already found many bugs following this exact construction.
With the obvious solution, there would be no problem :

	ret = read_next_word(hw, &word);
	if (!ret)
		return -EINVAL;

Best regards,
Willy

Re: A CodingStyle suggestion

[Ahmed S. Darwish]

On Sat, Feb 03, 2007 at 11:58:48PM +0200, Darwish wrote:
> Hi all,
> 
> 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 */
> }

Thanks for all the replies, A patch will be sent in a new thread.  

Regards
-- 
Ahmed S. Darwish
http://darwish-07.blogspot.com

Re: A CodingStyle suggestion

[Manu Abraham]

On 2/4/07, Ahmed S. Darwish <darwish.07@gmail.com> wrote:
> Hi all,
>
> 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) ...


if we have some 100 lines of

ret = do_method()
if (ret) {
/* error handling */
}

This is going to additionally increase the number of lines of unnecessarily.

IMHO, when you have a large number of lines which do a similar thing ..

if ((ret = do_method()) < value)
   goto err;

could greatly reduce the number of lines, otherwise.

manu

Re: A CodingStyle suggestion

[Theodore Tso]

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

		
		

Re: A CodingStyle suggestion

[Manu Abraham]

On 2/4/07, Theodore Tso <tytso@mit.edu> 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.


Completely agree, as i said in my earlier post.



manu

Re: A CodingStyle suggestion

[Robert P. J. Day]

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
========================================================================