* Re: [patch 3/3] fs/cramfs/inode.c: remove error variable
2007-08-30 15:37 [patch 3/3] fs/cramfs/inode.c: remove error variable Andi Drebes
@ 2007-08-31 11:43 ` Richard Knutsson
2007-08-31 16:12 ` Nishanth Aravamudan
` (5 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Richard Knutsson @ 2007-08-31 11:43 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [patch 3/3] fs/cramfs/inode.c: remove error variable
2007-08-30 15:37 [patch 3/3] fs/cramfs/inode.c: remove error variable Andi Drebes
2007-08-31 11:43 ` Richard Knutsson
@ 2007-08-31 16:12 ` Nishanth Aravamudan
2007-08-31 16:20 ` Andi Drebes
` (4 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Nishanth Aravamudan @ 2007-08-31 16:12 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [patch 3/3] fs/cramfs/inode.c: remove error variable
2007-08-30 15:37 [patch 3/3] fs/cramfs/inode.c: remove error variable Andi Drebes
2007-08-31 11:43 ` Richard Knutsson
2007-08-31 16:12 ` Nishanth Aravamudan
@ 2007-08-31 16:20 ` Andi Drebes
2007-08-31 16:23 ` Richard Knutsson
` (3 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Andi Drebes @ 2007-08-31 16:20 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [patch 3/3] fs/cramfs/inode.c: remove error variable
2007-08-30 15:37 [patch 3/3] fs/cramfs/inode.c: remove error variable Andi Drebes
` (2 preceding siblings ...)
2007-08-31 16:20 ` Andi Drebes
@ 2007-08-31 16:23 ` Richard Knutsson
2007-08-31 16:25 ` Randy Dunlap
` (2 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Richard Knutsson @ 2007-08-31 16:23 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [patch 3/3] fs/cramfs/inode.c: remove error variable
2007-08-30 15:37 [patch 3/3] fs/cramfs/inode.c: remove error variable Andi Drebes
` (3 preceding siblings ...)
2007-08-31 16:23 ` Richard Knutsson
@ 2007-08-31 16:25 ` Randy Dunlap
2007-08-31 16:35 ` Richard Knutsson
2007-08-31 16:49 ` Nishanth Aravamudan
6 siblings, 0 replies; 8+ messages in thread
From: Randy Dunlap @ 2007-08-31 16:25 UTC (permalink / raw)
To: kernel-janitors
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 ***
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [patch 3/3] fs/cramfs/inode.c: remove error variable
2007-08-30 15:37 [patch 3/3] fs/cramfs/inode.c: remove error variable Andi Drebes
` (4 preceding siblings ...)
2007-08-31 16:25 ` Randy Dunlap
@ 2007-08-31 16:35 ` Richard Knutsson
2007-08-31 16:49 ` Nishanth Aravamudan
6 siblings, 0 replies; 8+ messages in thread
From: Richard Knutsson @ 2007-08-31 16:35 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [patch 3/3] fs/cramfs/inode.c: remove error variable
2007-08-30 15:37 [patch 3/3] fs/cramfs/inode.c: remove error variable Andi Drebes
` (5 preceding siblings ...)
2007-08-31 16:35 ` Richard Knutsson
@ 2007-08-31 16:49 ` Nishanth Aravamudan
6 siblings, 0 replies; 8+ messages in thread
From: Nishanth Aravamudan @ 2007-08-31 16:49 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 8+ messages in thread