[PATCH] xfs: remove unused variable warning in xfs_finish_page_writeback

[PATCH] xfs: remove unused variable warning in xfs_finish_page_writeback

[Carlos Maiolino]

From: Carlos Maiolino <cmaiolino@redhat.com>

Due the initialization of blockmask variable with its definition, gcc issues the
following warning due an unused variable during compilation time.

fs/xfs//xfs_aops.c: In function ‘xfs_finish_page_writeback’:
fs/xfs//xfs_aops.c:97:15: warning: unused variable ‘blockmask’
[-Wunused-variable]
  unsigned int  blockmask = (1 << inode->i_blkbits) - 1;
               ^
Remove this warning by initializing the variable after its definition.

This change causes no difference in the generated assembly code

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/xfs_aops.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index 40645a4..f4e2d3b 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -94,11 +94,12 @@ xfs_finish_page_writeback(
 	struct bio_vec		*bvec,
 	int			error)
 {
-	unsigned int		blockmask = (1 << inode->i_blkbits) - 1;
+	unsigned int		blockmask;
 	unsigned int		end = bvec->bv_offset + bvec->bv_len - 1;
 	struct buffer_head	*head, *bh;
 	unsigned int		off = 0;
 
+	blockmask = (1 << inode->i_blkbits) - 1;
 	ASSERT(bvec->bv_offset < PAGE_SIZE);
 	ASSERT((bvec->bv_offset & blockmask) == 0);
 	ASSERT(end < PAGE_SIZE);
-- 
2.4.11

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: remove unused variable warning in xfs_finish_page_writeback

[Eric Sandeen]

On 5/11/16 11:14 AM, Carlos Maiolino wrote:
> From: Carlos Maiolino <cmaiolino@redhat.com>
> 
> Due the initialization of blockmask variable with its definition, gcc issues the
> following warning due an unused variable during compilation time.
> 
> fs/xfs//xfs_aops.c: In function ‘xfs_finish_page_writeback’:
> fs/xfs//xfs_aops.c:97:15: warning: unused variable ‘blockmask’
> [-Wunused-variable]
>   unsigned int  blockmask = (1 << inode->i_blkbits) - 1;
>                ^
> Remove this warning by initializing the variable after its definition.
> 
> This change causes no difference in the generated assembly code

Oh, I see.  It's unused on non-debug builds because it's only used
under ASSERT.

Ok, that's a good way to fix it, although there are some scanners
out there (clang?  some gcc?  I don't recall) which will also flag
write-only variables.  It'd be ugly but sometimes I wonder if an
ASSERT_DECL() macro would make some sense.

Or a MASK() macro so we could just do:

#define MASK(nbits) ((1UL << (nbits)) - 1) /* mask with NBITS bits set */

ASSERT((bvec->bv_offset & MASK(inode->i_blkbits)) == 0);

But for now, this is better, and fixes what most people will see,
so fine by me:

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  fs/xfs/xfs_aops.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index 40645a4..f4e2d3b 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -94,11 +94,12 @@ xfs_finish_page_writeback(
>  	struct bio_vec		*bvec,
>  	int			error)
>  {
> -	unsigned int		blockmask = (1 << inode->i_blkbits) - 1;
> +	unsigned int		blockmask;
>  	unsigned int		end = bvec->bv_offset + bvec->bv_len - 1;
>  	struct buffer_head	*head, *bh;
>  	unsigned int		off = 0;
>  
> +	blockmask = (1 << inode->i_blkbits) - 1;
>  	ASSERT(bvec->bv_offset < PAGE_SIZE);
>  	ASSERT((bvec->bv_offset & blockmask) == 0);
>  	ASSERT(end < PAGE_SIZE);
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: remove unused variable warning in xfs_finish_page_writeback

[Carlos Maiolino]

On Wed, May 11, 2016 at 11:33:21AM -0500, Eric Sandeen wrote:
> 
> 
> On 5/11/16 11:14 AM, Carlos Maiolino wrote:
> > From: Carlos Maiolino <cmaiolino@redhat.com>
> > 
> > Due the initialization of blockmask variable with its definition, gcc issues the
> > following warning due an unused variable during compilation time.
> > 
> > fs/xfs//xfs_aops.c: In function ‘xfs_finish_page_writeback’:
> > fs/xfs//xfs_aops.c:97:15: warning: unused variable ‘blockmask’
> > [-Wunused-variable]
> >   unsigned int  blockmask = (1 << inode->i_blkbits) - 1;
> >                ^
> > Remove this warning by initializing the variable after its definition.
> > 
> > This change causes no difference in the generated assembly code
> 
> Oh, I see.  It's unused on non-debug builds because it's only used
> under ASSERT.
> 
> Ok, that's a good way to fix it, although there are some scanners
> out there (clang?  some gcc?  I don't recall) which will also flag
> write-only variables.  It'd be ugly but sometimes I wonder if an
> ASSERT_DECL() macro would make some sense.
> 
> Or a MASK() macro so we could just do:
> 
> #define MASK(nbits) ((1UL << (nbits)) - 1) /* mask with NBITS bits set */
> 
> ASSERT((bvec->bv_offset & MASK(inode->i_blkbits)) == 0);
> 

I was wondering if we could use something like uninitialized_var() for such
cases, but, I think it's a sort of too extra complexity to just hide a variable
initialization in a different place than together with its definition. I really
don't think it's worth.

Even in the case you mentioned above, IMHO, MASK is going to hide something that
does not need to be hidden, only adding some extra jumps in my cscope to
understand what MASK is doing. Although, it might only be my lazy side saying
it :)

> But for now, this is better, and fixes what most people will see,
> so fine by me:
> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> 
> > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > ---
> >  fs/xfs/xfs_aops.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> > index 40645a4..f4e2d3b 100644
> > --- a/fs/xfs/xfs_aops.c
> > +++ b/fs/xfs/xfs_aops.c
> > @@ -94,11 +94,12 @@ xfs_finish_page_writeback(
> >  	struct bio_vec		*bvec,
> >  	int			error)
> >  {
> > -	unsigned int		blockmask = (1 << inode->i_blkbits) - 1;
> > +	unsigned int		blockmask;
> >  	unsigned int		end = bvec->bv_offset + bvec->bv_len - 1;
> >  	struct buffer_head	*head, *bh;
> >  	unsigned int		off = 0;
> >  
> > +	blockmask = (1 << inode->i_blkbits) - 1;
> >  	ASSERT(bvec->bv_offset < PAGE_SIZE);
> >  	ASSERT((bvec->bv_offset & blockmask) == 0);
> >  	ASSERT(end < PAGE_SIZE);
> > 
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

-- 
Carlos

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: remove unused variable warning in xfs_finish_page_writeback

[Christoph Hellwig]

On Wed, May 11, 2016 at 11:33:21AM -0500, Eric Sandeen wrote:
> Oh, I see.  It's unused on non-debug builds because it's only used
> under ASSERT.
> 
> Ok, that's a good way to fix it, although there are some scanners
> out there (clang?  some gcc?  I don't recall) which will also flag
> write-only variables.  It'd be ugly but sometimes I wonder if an
> ASSERT_DECL() macro would make some sense.
> 
> Or a MASK() macro so we could just do:
> 
> #define MASK(nbits) ((1UL << (nbits)) - 1) /* mask with NBITS bits set */

aka xfs_mask32lo from xfs_bit.h? :)

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs