[patch 3/3] fs/cramfs/inode.c: remove error variable

[patch 3/3] fs/cramfs/inode.c: remove error variable

[Andi Drebes]

This patch removes a variable from fs/cramfs/inode.c that is just used to store
a return value which is immediately read afterwards.

Tested on an i386 box.
Signed-off-by: Andi Drebes <lists-receive@programmierforen.de>
---
diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
index 350680f..42d2cf8 100644
--- a/fs/cramfs/inode.c
+++ b/fs/cramfs/inode.c
@@ -372,7 +372,7 @@ static int cramfs_readdir(struct file *filp, void *dirent, filldir_t filldir)
 		char *name;
 		ino_t ino;
 		mode_t mode;
-		int namelen, error;
+		int namelen;
 
 		mutex_lock(&read_mutex);
 		de = cramfs_read(sb, OFFSET(inode) + offset, sizeof(*de)+CRAMFS_MAXPATHLEN);
@@ -398,8 +398,7 @@ static int cramfs_readdir(struct file *filp, void *dirent, filldir_t filldir)
 				break;
 			namelen--;
 		}
-		error = filldir(dirent, buf, namelen, offset, ino, mode >> 12);
-		if (error)
+		if(filldir(dirent, buf, namelen, offset, ino, mode >> 12))
 			break;
 
 		offset = nextoffset;

Re: [patch 3/3] fs/cramfs/inode.c: remove error variable

[Richard Knutsson]

Andi Drebes wrote:
> This patch removes a variable from fs/cramfs/inode.c that is just used to store
> a return value which is immediately read afterwards.
>
> Tested on an i386 box.
> Signed-off-by: Andi Drebes <lists-receive@programmierforen.de>
> ---
> diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
> index 350680f..42d2cf8 100644
> --- a/fs/cramfs/inode.c
> +++ b/fs/cramfs/inode.c
> @@ -372,7 +372,7 @@ static int cramfs_readdir(struct file *filp, void *dirent, filldir_t filldir)
>  		char *name;
>  		ino_t ino;
>  		mode_t mode;
> -		int namelen, error;
> +		int namelen;
>  
>  		mutex_lock(&read_mutex);
>  		de = cramfs_read(sb, OFFSET(inode) + offset, sizeof(*de)+CRAMFS_MAXPATHLEN);
> @@ -398,8 +398,7 @@ static int cramfs_readdir(struct file *filp, void *dirent, filldir_t filldir)
>  				break;
>  			namelen--;
>  		}
> -		error = filldir(dirent, buf, namelen, offset, ino, mode >> 12);
> -		if (error)
> +		if(filldir(dirent, buf, namelen, offset, ino, mode >> 12))
>   
Maybe picky but please leave it as "if (".
Also, is it suppose to mean that if filldir() can do/execute 
(something), "break;" is going to happened? The 'error'-variable has the 
added value of telling the programmer it is an error-condition. So IMHO 
a "!= 0" would be appropriate (or in the range the error-values are in).

Richard Knutsson

Re: [patch 3/3] fs/cramfs/inode.c: remove error variable

[Nishanth Aravamudan]

On 31.08.2007 [13:43:18 +0200], Richard Knutsson wrote:
> Andi Drebes wrote:
> >This patch removes a variable from fs/cramfs/inode.c that is just used to 
> >store
> >a return value which is immediately read afterwards.
> >
> >Tested on an i386 box.
> >Signed-off-by: Andi Drebes <lists-receive@programmierforen.de>
> >---
> >diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
> >index 350680f..42d2cf8 100644
> >--- a/fs/cramfs/inode.c
> >+++ b/fs/cramfs/inode.c
> >@@ -372,7 +372,7 @@ static int cramfs_readdir(struct file *filp, void 
> >*dirent, filldir_t filldir)
> > 		char *name;
> > 		ino_t ino;
> > 		mode_t mode;
> >-		int namelen, error;
> >+		int namelen;
> > 
> > 		mutex_lock(&read_mutex);
> > 		de = cramfs_read(sb, OFFSET(inode) + offset, 
> > 		sizeof(*de)+CRAMFS_MAXPATHLEN);
> >@@ -398,8 +398,7 @@ static int cramfs_readdir(struct file *filp, void 
> >*dirent, filldir_t filldir)
> > 				break;
> > 			namelen--;
> > 		}
> >-		error = filldir(dirent, buf, namelen, offset, ino, mode >> 
> >12);
> >-		if (error)
> >+		if(filldir(dirent, buf, namelen, offset, ino, mode >> 12))
> >  
> Maybe picky but please leave it as "if (".

Beyond that, I just don't like this change. I thought the preferred way
for these types of statements was the previous version. That is:

error = filldir(...);
if (error)
	error stuff

Rather than a conditional with potential side-effects?

Thanks,
Nish

-- 
Nishanth Aravamudan <nacc@us.ibm.com>
IBM Linux Technology Center

Re: [patch 3/3] fs/cramfs/inode.c: remove error variable

[Andi Drebes]

Am Freitag, 31. August 2007 18:12 schrieb Nishanth Aravamudan:
> On 31.08.2007 [13:43:18 +0200], Richard Knutsson wrote:
> > Andi Drebes wrote:
> > >This patch removes a variable from fs/cramfs/inode.c that is just used to 
> > >store
> > >a return value which is immediately read afterwards.
> > >
> > >Tested on an i386 box.
> > >Signed-off-by: Andi Drebes <lists-receive@programmierforen.de>
> > >---
> > >diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
> > >index 350680f..42d2cf8 100644
> > >--- a/fs/cramfs/inode.c
> > >+++ b/fs/cramfs/inode.c
> > >@@ -372,7 +372,7 @@ static int cramfs_readdir(struct file *filp, void 
> > >*dirent, filldir_t filldir)
> > > 		char *name;
> > > 		ino_t ino;
> > > 		mode_t mode;
> > >-		int namelen, error;
> > >+		int namelen;
> > > 
> > > 		mutex_lock(&read_mutex);
> > > 		de = cramfs_read(sb, OFFSET(inode) + offset, 
> > > 		sizeof(*de)+CRAMFS_MAXPATHLEN);
> > >@@ -398,8 +398,7 @@ static int cramfs_readdir(struct file *filp, void 
> > >*dirent, filldir_t filldir)
> > > 				break;
> > > 			namelen--;
> > > 		}
> > >-		error = filldir(dirent, buf, namelen, offset, ino, mode >> 
> > >12);
> > >-		if (error)
> > >+		if(filldir(dirent, buf, namelen, offset, ino, mode >> 12))
> > >  
> > Maybe picky but please leave it as "if (".
> 
> Beyond that, I just don't like this change. I thought the preferred way
> for these types of statements was the previous version. That is:
> 
> error = filldir(...);
> if (error)
> 	error stuff
> 
> Rather than a conditional with potential side-effects?
OK. Maybe I exaggerated a little bit with this change. The most important
ones are those in patch 1/3 and 2/3. Maybe the best thing is to abolish the
last patch that deals with the error variable and to apply only the first two
patches.
I didn't really get why "if (foo)" is better than "if(foo)". Is it just a matter of style?

Thanks for your comments.

	cheers,
		Andi

Re: [patch 3/3] fs/cramfs/inode.c: remove error variable

[Richard Knutsson]

Nishanth Aravamudan wrote:
> On 31.08.2007 [13:43:18 +0200], Richard Knutsson wrote:
>   
>> Andi Drebes wrote:
>>     
>>> This patch removes a variable from fs/cramfs/inode.c that is just used to 
>>> store
>>> a return value which is immediately read afterwards.
>>>
>>> Tested on an i386 box.
>>> Signed-off-by: Andi Drebes <lists-receive@programmierforen.de>
>>> ---
>>> diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
>>> index 350680f..42d2cf8 100644
>>> --- a/fs/cramfs/inode.c
>>> +++ b/fs/cramfs/inode.c
>>> @@ -372,7 +372,7 @@ static int cramfs_readdir(struct file *filp, void 
>>> *dirent, filldir_t filldir)
>>> 		char *name;
>>> 		ino_t ino;
>>> 		mode_t mode;
>>> -		int namelen, error;
>>> +		int namelen;
>>>
>>> 		mutex_lock(&read_mutex);
>>> 		de = cramfs_read(sb, OFFSET(inode) + offset, 
>>> 		sizeof(*de)+CRAMFS_MAXPATHLEN);
>>> @@ -398,8 +398,7 @@ static int cramfs_readdir(struct file *filp, void 
>>> *dirent, filldir_t filldir)
>>> 				break;
>>> 			namelen--;
>>> 		}
>>> -		error = filldir(dirent, buf, namelen, offset, ino, mode >> 
>>> 12);
>>> -		if (error)
>>> +		if(filldir(dirent, buf, namelen, offset, ino, mode >> 12))
>>>  
>>>       
>> Maybe picky but please leave it as "if (".
>>     
>
> Beyond that, I just don't like this change. I thought the preferred way
> for these types of statements was the previous version. That is:
>
> error = filldir(...);
> if (error)
> 	error stuff
>
> Rather than a conditional with potential side-effects?
>   
Which side-effects are you thinking of? We quite often have:
if (!if_this_fails_it_all_fails())
    return -FAILED;

but we also have:
if ((p = malloc(...)) = NULL)
    ...

and that is a different story (IMHO).

But I don't mind either way...
(the compiler will get rid of it anyway, right?)

cu
Richard Knutsson

Re: [patch 3/3] fs/cramfs/inode.c: remove error variable

[Randy Dunlap]

On Fri, 31 Aug 2007 18:20:55 +0200 Andi Drebes wrote:

> Am Freitag, 31. August 2007 18:12 schrieb Nishanth Aravamudan:
> > On 31.08.2007 [13:43:18 +0200], Richard Knutsson wrote:
> > > Andi Drebes wrote:
> > > >This patch removes a variable from fs/cramfs/inode.c that is just used to 
> > > >store
> > > >a return value which is immediately read afterwards.
> > > >
> > > >Tested on an i386 box.
> > > >Signed-off-by: Andi Drebes <lists-receive@programmierforen.de>
> > > >---
> > > >diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
> > > >index 350680f..42d2cf8 100644
> > > >--- a/fs/cramfs/inode.c
> > > >+++ b/fs/cramfs/inode.c
> > > >@@ -372,7 +372,7 @@ static int cramfs_readdir(struct file *filp, void 
> > > >*dirent, filldir_t filldir)
> > > > 		char *name;
> > > > 		ino_t ino;
> > > > 		mode_t mode;
> > > >-		int namelen, error;
> > > >+		int namelen;
> > > > 
> > > > 		mutex_lock(&read_mutex);
> > > > 		de = cramfs_read(sb, OFFSET(inode) + offset, 
> > > > 		sizeof(*de)+CRAMFS_MAXPATHLEN);
> > > >@@ -398,8 +398,7 @@ static int cramfs_readdir(struct file *filp, void 
> > > >*dirent, filldir_t filldir)
> > > > 				break;
> > > > 			namelen--;
> > > > 		}
> > > >-		error = filldir(dirent, buf, namelen, offset, ino, mode >> 
> > > >12);
> > > >-		if (error)
> > > >+		if(filldir(dirent, buf, namelen, offset, ino, mode >> 12))
> > > >  
> > > Maybe picky but please leave it as "if (".
> > 
> > Beyond that, I just don't like this change. I thought the preferred way
> > for these types of statements was the previous version. That is:
> > 
> > error = filldir(...);
> > if (error)
> > 	error stuff
> > 
> > Rather than a conditional with potential side-effects?
> OK. Maybe I exaggerated a little bit with this change. The most important
> ones are those in patch 1/3 and 2/3. Maybe the best thing is to abolish the
> last patch that deals with the error variable and to apply only the first two
> patches.
> I didn't really get why "if (foo)" is better than "if(foo)". Is it just a matter of style?

Yes, it's style.  We generally treat asdf(foo) as function syntax,
so if, while, for, switch, etc., should be followed by a space before
the '('.


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [patch 3/3] fs/cramfs/inode.c: remove error variable

[Richard Knutsson]

Andi Drebes wrote:
> Am Freitag, 31. August 2007 18:12 schrieb Nishanth Aravamudan:
>   
>> On 31.08.2007 [13:43:18 +0200], Richard Knutsson wrote:
>>     
>>> Andi Drebes wrote:
>>>       
>>>> This patch removes a variable from fs/cramfs/inode.c that is just used to 
>>>> store
>>>> a return value which is immediately read afterwards.
>>>>
>>>> Tested on an i386 box.
>>>> Signed-off-by: Andi Drebes <lists-receive@programmierforen.de>
>>>> ---
>>>> diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
>>>> index 350680f..42d2cf8 100644
>>>> --- a/fs/cramfs/inode.c
>>>> +++ b/fs/cramfs/inode.c
>>>> @@ -372,7 +372,7 @@ static int cramfs_readdir(struct file *filp, void 
>>>> *dirent, filldir_t filldir)
>>>> 		char *name;
>>>> 		ino_t ino;
>>>> 		mode_t mode;
>>>> -		int namelen, error;
>>>> +		int namelen;
>>>>
>>>> 		mutex_lock(&read_mutex);
>>>> 		de = cramfs_read(sb, OFFSET(inode) + offset, 
>>>> 		sizeof(*de)+CRAMFS_MAXPATHLEN);
>>>> @@ -398,8 +398,7 @@ static int cramfs_readdir(struct file *filp, void 
>>>> *dirent, filldir_t filldir)
>>>> 				break;
>>>> 			namelen--;
>>>> 		}
>>>> -		error = filldir(dirent, buf, namelen, offset, ino, mode >> 
>>>> 12);
>>>> -		if (error)
>>>> +		if(filldir(dirent, buf, namelen, offset, ino, mode >> 12))
>>>>  
>>>>         
>>> Maybe picky but please leave it as "if (".
>>>       
>> Beyond that, I just don't like this change. I thought the preferred way
>> for these types of statements was the previous version. That is:
>>
>> error = filldir(...);
>> if (error)
>> 	error stuff
>>
>> Rather than a conditional with potential side-effects?
>>     
> OK. Maybe I exaggerated a little bit with this change. The most important
> ones are those in patch 1/3 and 2/3. Maybe the best thing is to abolish the
> last patch that deals with the error variable and to apply only the first two
> patches.
>   
Better/easier to do too much and cut it down a little then the other way 
around. ;)
> I didn't really get why "if (foo)" is better than "if(foo)". Is it just a matter of style?
>   
See the all mighty CodingStyle on 3.1
(however, it should not be taken too far but I don't see the advantage 
to make it "if(")

cu
Richard Knutsson

Re: [patch 3/3] fs/cramfs/inode.c: remove error variable

[Nishanth Aravamudan]

On 31.08.2007 [18:23:40 +0200], Richard Knutsson wrote:
> Nishanth Aravamudan wrote:
> >On 31.08.2007 [13:43:18 +0200], Richard Knutsson wrote:
> >  
> >>Andi Drebes wrote:
> >>    
> >>>This patch removes a variable from fs/cramfs/inode.c that is just used 
> >>>to store
> >>>a return value which is immediately read afterwards.
> >>>
> >>>Tested on an i386 box.
> >>>Signed-off-by: Andi Drebes <lists-receive@programmierforen.de>
> >>>---
> >>>diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
> >>>index 350680f..42d2cf8 100644
> >>>--- a/fs/cramfs/inode.c
> >>>+++ b/fs/cramfs/inode.c
> >>>@@ -372,7 +372,7 @@ static int cramfs_readdir(struct file *filp, void 
> >>>*dirent, filldir_t filldir)
> >>>		char *name;
> >>>		ino_t ino;
> >>>		mode_t mode;
> >>>-		int namelen, error;
> >>>+		int namelen;
> >>>
> >>>		mutex_lock(&read_mutex);
> >>>		de = cramfs_read(sb, OFFSET(inode) + offset, 
> >>>		sizeof(*de)+CRAMFS_MAXPATHLEN);
> >>>@@ -398,8 +398,7 @@ static int cramfs_readdir(struct file *filp, void 
> >>>*dirent, filldir_t filldir)
> >>>				break;
> >>>			namelen--;
> >>>		}
> >>>-		error = filldir(dirent, buf, namelen, offset, ino, mode >> 
> >>>12);
> >>>-		if (error)
> >>>+		if(filldir(dirent, buf, namelen, offset, ino, mode >> 12))
> >>> 
> >>>      
> >>Maybe picky but please leave it as "if (".
> >>    
> >
> >Beyond that, I just don't like this change. I thought the preferred way
> >for these types of statements was the previous version. That is:
> >
> >error = filldir(...);
> >if (error)
> >	error stuff
> >
> >Rather than a conditional with potential side-effects?
> >  
> Which side-effects are you thinking of? We quite often have:
> if (!if_this_fails_it_all_fails())
>    return -FAILED;
> 
> but we also have:
> if ((p = malloc(...)) = NULL)
>    ...
> 
> and that is a different story (IMHO).
> 
> But I don't mind either way...
> (the compiler will get rid of it anyway, right?)

There may not be side-effects here, you're right. And maybe I'm wrong.
But I also think code generally can be easier to read in the original
formatting. I would be curious to see the effect on the end-compiled
code, actually -- it does seem like the compiler should have some smarts
here.

Thanks,
Nish

-- 
Nishanth Aravamudan <nacc@us.ibm.com>
IBM Linux Technology Center