Re: [PATCH v2 8/9] bfs: remove multiple assignments
Al Viro wrote: > On Mon, Jan 28, 2008 at 01:02:03AM -0600, Joel Schopp wrote: > -inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME_SEC; > +inode->i_mtime = CURRENT_TIME_SEC; > +inode->i_atime = CURRENT_TIME_SEC; > +inode->i_ctime = CURRENT_TIME_SEC; multiple assignments like "x = y = z = value;" can potentially (depending on the compiler and arch) be faster than "x = value; y = value; z=value;" I am surprized that this script complains about them as it is a perfectly valid thing to do in C. >>> I think it seems wise to ask the maintainers of checkpatch.pl to >>> comment on that. I'm Cc:ing them now. >>> >> There are plenty of things that are valid to do in C that don't make for >> maintainable code. These scripts are designed to make your code easier for >> real people to review and maintain. > > Except that in this case the new variant is not equivalent to the old one... Yes, you're right. In fact, I felt like sending yet another version of these patches, but this gets preempted all the time by "the other things". Dmitri - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 8/9] bfs: remove multiple assignments
On Mon, Jan 28, 2008 at 01:02:03AM -0600, Joel Schopp wrote: > >>>-inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME_SEC; > >>>+inode->i_mtime = CURRENT_TIME_SEC; > >>>+inode->i_atime = CURRENT_TIME_SEC; > >>>+inode->i_ctime = CURRENT_TIME_SEC; > >>multiple assignments like "x = y = z = value;" can potentially > >>(depending on the compiler and arch) be faster than "x = value; y = > >>value; z=value;" > >> > >>I am surprized that this script complains about them as it is a > >>perfectly valid thing to do in C. > > > >I think it seems wise to ask the maintainers of checkpatch.pl to > >comment on that. I'm Cc:ing them now. > > > > There are plenty of things that are valid to do in C that don't make for > maintainable code. These scripts are designed to make your code easier for > real people to review and maintain. Except that in this case the new variant is not equivalent to the old one... - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 8/9] bfs: remove multiple assignments
-inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME_SEC; +inode->i_mtime = CURRENT_TIME_SEC; +inode->i_atime = CURRENT_TIME_SEC; +inode->i_ctime = CURRENT_TIME_SEC; multiple assignments like "x = y = z = value;" can potentially (depending on the compiler and arch) be faster than "x = value; y = value; z=value;" I am surprized that this script complains about them as it is a perfectly valid thing to do in C. I think it seems wise to ask the maintainers of checkpatch.pl to comment on that. I'm Cc:ing them now. There are plenty of things that are valid to do in C that don't make for maintainable code. These scripts are designed to make your code easier for real people to review and maintain. As for if this can be faster we don't deal in the realm of "can". Please show a concrete example of gcc making Linux kernel code faster with multiple assignments per line. If you can do that I'm willing to change my mind and I'll lead the charge for mutliple assignments per line. - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 8/9] bfs: remove multiple assignments
Adrian Bunk пишет: > On Sat, Jan 26, 2008 at 06:35:41PM +, Tigran Aivazian wrote: >> On Sat, 26 Jan 2008, Dmitri Vorobiev wrote: >>> - inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME_SEC; >>> + inode->i_mtime = CURRENT_TIME_SEC; >>> + inode->i_atime = CURRENT_TIME_SEC; >>> + inode->i_ctime = CURRENT_TIME_SEC; >> multiple assignments like "x = y = z = value;" can potentially (depending >> on the compiler and arch) be faster than "x = value; y = value; z=value;" > > Only depending on the compiler, and recent gcc versions are quite good > at optimizing code. > >> I am surprized that this script complains about them as it is a perfectly >> valid thing to do in C. > > Checkpatch warns about the something already documented in > Documentation/CodingStyle: > > <-- snip --> > > Don't put multiple assignments on a single line either. Kernel coding style > is super simple. Avoid tricky expressions. > > <-- snip --> > > Nobody claims it wasn't perfectly valid C code, the point is that > multiple assignments on a single line are harder to read. Thanks for your explanation. So, it was just a matter of readability, not language lawyering. In fact, this is how I interpreted that complaint from the checkpatch.pl script. Dmitri - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 8/9] bfs: remove multiple assignments
On Sat, Jan 26, 2008 at 06:35:41PM +, Tigran Aivazian wrote: > On Sat, 26 Jan 2008, Dmitri Vorobiev wrote: >> -inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME_SEC; >> +inode->i_mtime = CURRENT_TIME_SEC; >> +inode->i_atime = CURRENT_TIME_SEC; >> +inode->i_ctime = CURRENT_TIME_SEC; > > multiple assignments like "x = y = z = value;" can potentially (depending > on the compiler and arch) be faster than "x = value; y = value; z=value;" Only depending on the compiler, and recent gcc versions are quite good at optimizing code. > I am surprized that this script complains about them as it is a perfectly > valid thing to do in C. Checkpatch warns about the something already documented in Documentation/CodingStyle: <-- snip --> Don't put multiple assignments on a single line either. Kernel coding style is super simple. Avoid tricky expressions. <-- snip --> Nobody claims it wasn't perfectly valid C code, the point is that multiple assignments on a single line are harder to read. > But this is not a "big deal", so I don't mind. > > Kind regards > Tigran cu Adrian -- "Is there not promise of rain?" Ling Tan asked suddenly out of the darkness. There had been need of rain for many days. "Only a promise," Lao Er said. Pearl S. Buck - Dragon Seed - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 8/9] bfs: remove multiple assignments
Tigran Aivazian wrote: > On Sat, 26 Jan 2008, Dmitri Vorobiev wrote: >> -inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME_SEC; >> +inode->i_mtime = CURRENT_TIME_SEC; >> +inode->i_atime = CURRENT_TIME_SEC; >> +inode->i_ctime = CURRENT_TIME_SEC; > > multiple assignments like "x = y = z = value;" can potentially > (depending on the compiler and arch) be faster than "x = value; y = > value; z=value;" > > I am surprized that this script complains about them as it is a > perfectly valid thing to do in C. I think it seems wise to ask the maintainers of checkpatch.pl to comment on that. I'm Cc:ing them now. Thanks, Dmitri - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 8/9] bfs: remove multiple assignments
Tigran Aivazian wrote: > On Sat, 26 Jan 2008, Dmitri Vorobiev wrote: >> -inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME_SEC; >> +inode->i_mtime = CURRENT_TIME_SEC; >> +inode->i_atime = CURRENT_TIME_SEC; >> +inode->i_ctime = CURRENT_TIME_SEC; > > multiple assignments like "x = y = z = value;" can potentially > (depending on the compiler and arch) be faster than "x = value; y = > value; z=value;" > > I am surprized that this script complains about them as it is a > perfectly valid thing to do in C. > Thanks for your comment. My goal was to make the driver "checkpatch.pl-clean", and that was the only reason for this change. I just assumed that what the script, which is recommended for use to check the kernel coding style, insists upon, gives an idea of what "the right way" is. >From the viewpoint of code clarity, it seems to be a matter of personal opinion whether multiple assignments should be preferred over giving a separate line to each variable. I personally prefer separated assignments, but in this particular case I would never touch these lines had the script not complained about it. > But this is not a "big deal", so I don't mind. > OK. Let's wait for Adrian to pick up these patches and push these into his tree. Thanks, Dmitri - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 8/9] bfs: remove multiple assignments
On Sat, 26 Jan 2008, Dmitri Vorobiev wrote: - inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME_SEC; + inode->i_mtime = CURRENT_TIME_SEC; + inode->i_atime = CURRENT_TIME_SEC; + inode->i_ctime = CURRENT_TIME_SEC; multiple assignments like "x = y = z = value;" can potentially (depending on the compiler and arch) be faster than "x = value; y = value; z=value;" I am surprized that this script complains about them as it is a perfectly valid thing to do in C. But this is not a "big deal", so I don't mind. Kind regards Tigran - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 8/9] bfs: remove multiple assignments
The checkpatch.pl reported several warnings about multiple variable
assignments in the BFS driver sources. This trivial patch fixes these
warnings by giving each assignment its own line.
No functional changes introduced.
This patch was compile-tested by building the BFS driver both
as a module and as a part of the kernel proper.
Signed-off-by: Dmitri Vorobiev <[EMAIL PROTECTED]>
Acked-by: Tigran Aivazian <[EMAIL PROTECTED]>
---
fs/bfs/dir.c | 13 +
fs/bfs/file.c |6 --
fs/bfs/inode.c |7 +--
3 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/fs/bfs/dir.c b/fs/bfs/dir.c
index 2964505..94fed7a 100644
--- a/fs/bfs/dir.c
+++ b/fs/bfs/dir.c
@@ -104,7 +104,9 @@ static int bfs_create(struct inode *dir, struct dentry
*dentry, int mode,
info->si_freei--;
inode->i_uid = current->fsuid;
inode->i_gid = (dir->i_mode & S_ISGID) ? dir->i_gid : current->fsgid;
- inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME_SEC;
+ inode->i_mtime = CURRENT_TIME_SEC;
+ inode->i_atime = CURRENT_TIME_SEC;
+ inode->i_ctime = CURRENT_TIME_SEC;
inode->i_blocks = 0;
inode->i_op = &bfs_file_inops;
inode->i_fop = &bfs_file_operations;
@@ -200,7 +202,8 @@ static int bfs_unlink(struct inode *dir, struct dentry
*dentry)
}
de->ino = 0;
mark_buffer_dirty(bh);
- dir->i_ctime = dir->i_mtime = CURRENT_TIME_SEC;
+ dir->i_ctime = CURRENT_TIME_SEC;
+ dir->i_mtime = CURRENT_TIME_SEC;
mark_inode_dirty(dir);
inode->i_ctime = dir->i_ctime;
inode_dec_link_count(inode);
@@ -220,7 +223,8 @@ static int bfs_rename(struct inode *old_dir, struct dentry
*old_dentry,
struct bfs_dirent *old_de, *new_de;
int error = -ENOENT;
- old_bh = new_bh = NULL;
+ old_bh = NULL;
+ new_bh = NULL;
old_inode = old_dentry->d_inode;
if (S_ISDIR(old_inode->i_mode))
return -EINVAL;
@@ -252,7 +256,8 @@ static int bfs_rename(struct inode *old_dir, struct dentry
*old_dentry,
goto end_rename;
}
old_de->ino = 0;
- old_dir->i_ctime = old_dir->i_mtime = CURRENT_TIME_SEC;
+ old_dir->i_ctime = CURRENT_TIME_SEC;
+ old_dir->i_mtime = CURRENT_TIME_SEC;
mark_inode_dirty(old_dir);
if (new_inode) {
new_inode->i_ctime = CURRENT_TIME_SEC;
diff --git a/fs/bfs/file.c b/fs/bfs/file.c
index f32b2a2..ab2fa66 100644
--- a/fs/bfs/file.c
+++ b/fs/bfs/file.c
@@ -111,7 +111,8 @@ static int bfs_get_block(struct inode *inode, sector_t
block,
create, (unsigned long)block, phys);
map_bh(bh_result, sb, phys);
info->si_freeb -= phys - bi->i_eblock;
- info->si_lf_eblk = bi->i_eblock = phys;
+ info->si_lf_eblk = phys;
+ bi->i_eblock = phys;
mark_inode_dirty(inode);
mark_buffer_dirty(sbh);
err = 0;
@@ -140,7 +141,8 @@ static int bfs_get_block(struct inode *inode, sector_t
block,
create, (unsigned long)block, phys);
bi->i_sblock = phys;
phys += block;
- info->si_lf_eblk = bi->i_eblock = phys;
+ info->si_lf_eblk = phys;
+ bi->i_eblock = phys;
/*
* This assumes nothing can write the inode back while we are here
diff --git a/fs/bfs/inode.c b/fs/bfs/inode.c
index 91d5686..7eefafb 100644
--- a/fs/bfs/inode.c
+++ b/fs/bfs/inode.c
@@ -157,7 +157,9 @@ static void bfs_delete_inode(struct inode *inode)
}
inode->i_size = 0;
- inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME_SEC;
+ inode->i_atime = CURRENT_TIME_SEC;
+ inode->i_mtime = CURRENT_TIME_SEC;
+ inode->i_ctime = CURRENT_TIME_SEC;
lock_kernel();
mark_inode_dirty(inode);
@@ -213,7 +215,8 @@ static int bfs_statfs(struct dentry *dentry, struct kstatfs
*buf)
buf->f_type = BFS_MAGIC;
buf->f_bsize = s->s_blocksize;
buf->f_blocks = info->si_blocks;
- buf->f_bfree = buf->f_bavail = info->si_freeb;
+ buf->f_bfree = info->si_freeb;
+ buf->f_bavail = info->si_freeb;
buf->f_files = info->si_lasti + 1 - BFS_ROOT_INO;
buf->f_ffree = info->si_freei;
buf->f_fsid.val[0] = (u32)id;
--
1.5.3
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
