[PATCH] staging: rtl8188eu: Macro should be enclosed

Sebastian Haas sehaas at deebas.com
Tue Apr 11 21:31:03 UTC 2017


On Sun, Apr 09, 2017 at 02:47:45PM +0100, alfonsolimaastor at gmail.com wrote:
> On Sat, Apr 01, 2017 at 09:24:58AM +0200, Sebastian Haas wrote:
> > Hi Alfonso,
> > 
> > On Wed, Mar 29, 2017 at 07:50:11PM +0100, alfonsolimaastor at gmail.com wrote:
> > > diff --git a/drivers/staging/rtl8188eu/include/odm_debug.h b/drivers/staging/rtl8188eu/include/odm_debug.h
> > > index 687ff3e..fd92f7e 100644
> > > --- a/drivers/staging/rtl8188eu/include/odm_debug.h
> > > +++ b/drivers/staging/rtl8188eu/include/odm_debug.h
> > > @@ -86,11 +86,13 @@
> > >  #endif
> > >  
> > >  #define ODM_RT_TRACE(pDM_Odm, comp, level, fmt)				\
> > > -	if (((comp) & pDM_Odm->DebugComponents) &&			\
> > > -	    (level <= pDM_Odm->DebugLevel)) {				\
> > > -		pr_info("[ODM-8188E] ");				\
> > > -		RT_PRINTK fmt;						\
> > > -	}
> > > +	do {								\
> > > +		if (((comp) & pDM_Odm->DebugComponents) &&		\
> > > +		    (level <= pDM_Odm->DebugLevel)) {			\
> > > +			pr_info("[ODM-8188E] ");			\
> > > +			RT_PRINTK fmt;					\
> > > +		}							\
> > > +	} while (0)
> > 
> > isn't the if-statement already a single block?
> > I don't think the do-while adds any improvement.
> 
> I have investigated a little bit and found no reason why checkpatch
> complains about this, so I reported a bug:
> http://www.spinics.net/lists/kernel/msg2484676.html
> 
> It turns out the problem comes when you use that macro inside an if-else
> statement like this:
> 
> if (foo())
> 	ODM_RT_TRACE(pDM_Odm, comp, level, fmt);
> else
> 	do_smomething();
> 
> So, checkpatch's complain seems to be legit.
> 
> Regards,
> Alfonso

Thanks for pointing that out. I have missed that 'else' scenario.
So, checkpatch was right and the patch is fine.

But I have a question about your example:
Wouldn't the original macro called with a trailing semicolon lead to a compile error?
On the other hand, a while-wrapped macro without semicolon would also result in an error.

/sebastian


More information about the devel mailing list