Re: [PATCH] nilfs2: trivial coding style fix

[Jiro SEKIBA <jir-hfpbi5WX9J54Eiagz67IpQ@public.gmane.org>]

Hi,

Thank you for the reveiew.
I'll revise the patch!

thanks

regards,

At Fri, 04 Dec 2009 20:01:02 +0900 (JST),
Ryusuke Konishi wrote:
> 
> Hi,
> On Fri, 04 Dec 2009 18:23:30 +0900, Jiro SEKIBA <jir-hfpbi5WX9J54Eiagz67IpQ@public.gmane.org> wrote:
> > Hi,
> > 
> > This is a trivial style fix patch to mend errors/warnings
> > reported by "checkpatch.pl --file".
> > 
> > Signed-off-by: Jiro SEKIBA <jir-hfpbi5WX9J54Eiagz67IpQ@public.gmane.org>
> > ---
> >  fs/nilfs2/bmap.c   |    3 ++-
> >  fs/nilfs2/cpfile.c |    5 +++--
> >  fs/nilfs2/direct.c |   14 ++++++++------
> >  3 files changed, 13 insertions(+), 9 deletions(-)
> 
> Ok, I'll comment inline below.
>  
> > diff --git a/fs/nilfs2/bmap.c b/fs/nilfs2/bmap.c
> > index 08834df..78ccbba 100644
> > --- a/fs/nilfs2/bmap.c
> > +++ b/fs/nilfs2/bmap.c
> > @@ -426,7 +426,8 @@ __u64 nilfs_bmap_data_get_key(const struct nilfs_bmap *bmap,
> >  	key = page_index(bh->b_page) << (PAGE_CACHE_SHIFT -
> >  					 bmap->b_inode->i_blkbits);
> >  	for (pbh = page_buffers(bh->b_page); pbh != bh;
> > -	     pbh = pbh->b_this_page, key++);
> > +	     pbh = pbh->b_this_page, key++)
> > +		;
> 
> How about moving the "key++" inside the loop?  The line break in the
> middle of the for-sentence looks eliminable.
> 
> >  	return key;
> >  }
> > diff --git a/fs/nilfs2/cpfile.c b/fs/nilfs2/cpfile.c
> > index 3f5d5d0..a86d402 100644
> > --- a/fs/nilfs2/cpfile.c
> > +++ b/fs/nilfs2/cpfile.c
> > @@ -328,9 +328,10 @@ int nilfs_cpfile_delete_checkpoints(struct inode *cpfile,
> >  			tnicps += nicps;
> >  			nilfs_mdt_mark_buffer_dirty(cp_bh);
> >  			nilfs_mdt_mark_dirty(cpfile);
> > +			count = nilfs_cpfile_block_sub_valid_checkpoints(
> > +				  cpfile, cp_bh, kaddr, nicps);
> >  			if (!nilfs_cpfile_is_in_first(cpfile, cno) &&
> > -			    (count = nilfs_cpfile_block_sub_valid_checkpoints(
> > -				    cpfile, cp_bh, kaddr, nicps)) == 0) {
> > +			    count == 0) {
> 
> This conversion is not equivalent because
> nilfs_cpfile_block_sub_valid_checkpoints() has a side effect.
> 
> >  				/* make hole */
> >  				kunmap_atomic(kaddr, KM_USER0);
> >  				brelse(cp_bh);
> > diff --git a/fs/nilfs2/direct.c b/fs/nilfs2/direct.c
> > index d369ac7..f817282 100644
> > --- a/fs/nilfs2/direct.c
> > +++ b/fs/nilfs2/direct.c
> > @@ -53,9 +53,10 @@ static int nilfs_direct_lookup(const struct nilfs_bmap *bmap,
> >  
> >  	direct = (struct nilfs_direct *)bmap;
> >  	if ((key > NILFS_DIRECT_KEY_MAX) ||
> > -	    (level != 1) ||	/* XXX: use macro for level 1 */
> > -	    ((ptr = nilfs_direct_get_ptr(direct, key)) ==
> > -	     NILFS_BMAP_INVALID_PTR))
> > +	    (level != 1))  /* XXX: use macro for level 1 */
> > +		return -ENOENT;
> > +	ptr = nilfs_direct_get_ptr(direct, key);
> > +	if (ptr == NILFS_BMAP_INVALID_PTR)
> >  		return -ENOENT;
> 
> Looks good, but it's still redundant.  How about removing unnecessary
> parentheses in this occasion?
> 
> The first conditional statement can be simplified to:
> 
> 	if (key > NILFS_DIRECT_KEY_MAX || level != 1)
> 
>   
> >  	if (ptrp != NULL)
> > @@ -73,9 +74,10 @@ static int nilfs_direct_lookup_contig(const struct nilfs_bmap *bmap,
> >  	sector_t blocknr;
> >  	int ret, cnt;
> >  
> > -	if (key > NILFS_DIRECT_KEY_MAX ||
> > -	    (ptr = nilfs_direct_get_ptr(direct, key)) ==
> > -	    NILFS_BMAP_INVALID_PTR)
> > +	if (key > NILFS_DIRECT_KEY_MAX)
> > +		return -ENOENT;
> > +	ptr = nilfs_direct_get_ptr(direct, key);
> > +	if (ptr == NILFS_BMAP_INVALID_PTR)
> >  		return -ENOENT;
> 
> Looks good to me.
>   
> >  	if (NILFS_BMAP_USE_VBN(bmap)) {
> > -- 
> > 1.5.6.5
> 
> Thanks,
> Ryusuke Konishi
> _______________________________________________
> users mailing list
> users-JrjvKiOkagjYtjvyW6yDsg@public.gmane.org
> https://www.nilfs.org/mailman/listinfo/users
> 
> 
> 

-- 
Jiro SEKIBA <jir-hfpbi5WX9J54Eiagz67IpQ@public.gmane.org>