fs/cramfs/inode.c: remove error variable

PATCH In review Andi Drebes 4 participants 8 messages Aug 30, 2007 – Aug 31, 2007

Subsystems (2)

Kernel janitors Cramfs filesystem

lore

Share & tools

lore

Andi Drebes

author

2007-08-30 15:37 UTC #0 lore raw

PATCH 3/3 fs/cramfs/inode.c: remove error variable

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 <[email protected]>

---

fs/cramfs/inode.c +2 -3

index 350680f..42d2cf8 100644
@@ -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;

Richard Knutsson

2007-08-31 11:43 UTC #1 lore raw

Quoted 20 lines · 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 <[email protected]>
---
fs/cramfs/inode.c +1 -1
index 350680f..42d2cf8 100644
@@ -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);

fs/cramfs/inode.c static int cramfs_readdir(struct file *filp, void *dirent, filldir_t filldir) · line 402

Quoted 7 lines

fs/cramfs/inode.c +1 -2

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

Nishanth Aravamudan

2007-08-31 16:12 UTC #2 lore raw

In reply to Andi Drebes

Quoted 20 lines · Richard Knutsson

Quoted 19 lines · 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 <[email protected]>
---
fs/cramfs/inode.c +1 -1
index 350680f..42d2cf8 100644
@@ -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;
 

Quoted 14 lines

fs/cramfs/inode.c static int cramfs_readdir(struct file *filp, void · line 404

Quoted 13 lines

fs/cramfs/inode.c +0 -0

 		mutex_lock(&read_mutex);

		de = cramfs_read(sb, OFFSET(inode) + offset, 
		sizeof(*de)+CRAMFS_MAXPATHLEN);

fs/cramfs/inode.c +1 -2

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

Signature · 2 lines

Nishanth Aravamudan <[email protected]>
IBM Linux Technology Center

Andi Drebes

author

2007-08-31 16:20 UTC #3 lore raw

In reply to Andi Drebes

Am Freitag, 31. August 2007 18:12 schrieb Nishanth Aravamudan:

Quoted 36 lines · Richard Knutsson

Quoted 20 lines
Quoted 19 lines · 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 <[email protected]>
---
fs/cramfs/inode.c +1 -1
index 350680f..42d2cf8 100644
@@ -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;
 
Quoted 14 lines
fs/cramfs/inode.c static int cramfs_readdir(struct file *filp, void · line 404
Quoted 13 lines
fs/cramfs/inode.c +0 -0
 		mutex_lock(&read_mutex);
		de = cramfs_read(sb, OFFSET(inode) + offset, 
		sizeof(*de)+CRAMFS_MAXPATHLEN);
fs/cramfs/inode.c +1 -2
@@ -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 (".

Quoted 8 lines

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

Richard Knutsson

2007-08-31 16:23 UTC #4 lore raw

In reply to Andi Drebes

Quoted 40 lines · Nishanth Aravamudan

Quoted 21 lines · Richard Knutsson
Quoted 19 lines · 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 <[email protected]>
---
fs/cramfs/inode.c +1 -1
index 350680f..42d2cf8 100644
@@ -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;
 
Quoted 16 lines
Quoted 5 lines
fs/cramfs/inode.c +0 -0
 		mutex_lock(&read_mutex);
		de = cramfs_read(sb, OFFSET(inode) + offset, 
		sizeof(*de)+CRAMFS_MAXPATHLEN);
fs/cramfs/inode.c static int cramfs_readdir(struct file *filp, void · line 404
Quoted 9 lines
fs/cramfs/inode.c +1 -2
@@ -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 (".

Quoted 9 lines

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

Randy Dunlap

2007-08-31 16:25 UTC #5 lore raw

In reply to Andi Drebes

Quoted 44 lines · On Fri, 31 Aug 2007 18:20:55 +0200 Andi Drebes wrote:

Am Freitag, 31. August 2007 18:12 schrieb Nishanth Aravamudan:
Quoted 43 lines · Richard Knutsson
Quoted 20 lines
Quoted 19 lines · 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 <[email protected]>
---
fs/cramfs/inode.c +1 -1
index 350680f..42d2cf8 100644
@@ -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;
 
Quoted 14 lines
fs/cramfs/inode.c static int cramfs_readdir(struct file *filp, void · line 404
Quoted 13 lines
fs/cramfs/inode.c +0 -0
 		mutex_lock(&read_mutex);
		de = cramfs_read(sb, OFFSET(inode) + offset, 
		sizeof(*de)+CRAMFS_MAXPATHLEN);
fs/cramfs/inode.c +1 -2
@@ -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

Quoted 6 lines

Quoted 1 line
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 ***

Richard Knutsson

2007-08-31 16:35 UTC #6 lore raw

In reply to Andi Drebes

Quoted 48 lines · Andi Drebes

Am Freitag, 31. August 2007 18:12 schrieb Nishanth Aravamudan:
Quoted 46 lines · Richard Knutsson
Quoted 22 lines
Quoted 19 lines · 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 <[email protected]>
---
fs/cramfs/inode.c +1 -1
index 350680f..42d2cf8 100644
@@ -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;
 
Quoted 16 lines
Quoted 5 lines
fs/cramfs/inode.c +0 -0
 		mutex_lock(&read_mutex);
		de = cramfs_read(sb, OFFSET(inode) + offset, 
		sizeof(*de)+CRAMFS_MAXPATHLEN);
fs/cramfs/inode.c static int cramfs_readdir(struct file *filp, void · line 404
Quoted 9 lines
fs/cramfs/inode.c +1 -2
@@ -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

Quoted 7 lines

Quoted 2 lines
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. ;)

Quoted 2 lines

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

Nishanth Aravamudan

2007-08-31 16:49 UTC #7 lore raw

In reply to Andi Drebes

Quoted 54 lines · Richard Knutsson

Quoted 40 lines · Nishanth Aravamudan
Quoted 21 lines · Richard Knutsson
Quoted 19 lines · 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 <[email protected]>
---
fs/cramfs/inode.c +1 -1
index 350680f..42d2cf8 100644
@@ -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;
 
Quoted 16 lines
Quoted 5 lines
fs/cramfs/inode.c +0 -0
 		mutex_lock(&read_mutex);
		de = cramfs_read(sb, OFFSET(inode) + offset, 
		sizeof(*de)+CRAMFS_MAXPATHLEN);
fs/cramfs/inode.c static int cramfs_readdir(struct file *filp, void · line 404
Quoted 9 lines
fs/cramfs/inode.c +1 -2
@@ -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 (".
Quoted 9 lines
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;

Quoted 8 lines

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

Signature · 2 lines

Nishanth Aravamudan <[email protected]>
IBM Linux Technology Center