Re: [Outreachy kernel] Re: [PATCH] Staging: rtl8192e: Remove ternary operator

[Greg KH <gregkh@linuxfoundation.org>]

On Sun, Oct 09, 2016 at 04:36:34PM +0200, Julia Lawall wrote:
> 
> 
> On Sun, 9 Oct 2016, Greg KH wrote:
> 
> > On Sat, Oct 08, 2016 at 11:58:17PM +0530, Bhumika Goyal wrote:
> > > Relational and logical operators evaluate to either true or false.
> > > Explicit conversion is not needed so remove the ternary operator.
> > > Done using coccinelle:
> > >
> > > @r@
> > > expression A,B;
> > > symbol true,false;
> > > binary operator b = {==,!=,&&,||,>=,<=,>,<};
> > > @@
> > > - (A b B) ? true : false
> > > + A b B
> > >
> > > Signed-off-by: Bhumika Goyal <bhumirks@gmail.com>
> > > ---
> > >  drivers/staging/rtl8192e/rtl819x_HTProc.c | 12 ++++--------
> > >  drivers/staging/rtl8192e/rtllib.h         |  2 +-
> > >  2 files changed, 5 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/staging/rtl8192e/rtl819x_HTProc.c b/drivers/staging/rtl8192e/rtl819x_HTProc.c
> > > index dd9c0c8..8cd51a9 100644
> > > --- a/drivers/staging/rtl8192e/rtl819x_HTProc.c
> > > +++ b/drivers/staging/rtl8192e/rtl819x_HTProc.c
> > > @@ -558,19 +558,15 @@ void HTOnAssocRsp(struct rtllib_device *ieee)
> > >  #endif
> > >  	HTSetConnectBwMode(ieee, (enum ht_channel_width)(pPeerHTCap->ChlWidth),
> > >  			  (enum ht_extchnl_offset)(pPeerHTInfo->ExtChlOffset));
> > > -	pHTInfo->bCurTxBW40MHz = ((pPeerHTInfo->RecommemdedTxWidth == 1) ?
> > > -				 true : false);
> > > +	pHTInfo->bCurTxBW40MHz = (pPeerHTInfo->RecommemdedTxWidth == 1);
> >
> > Ugh, I _hate_ ? : statements.
> >
> > Just write the thing out, is this easy to read as is?
> >
> > >  	pHTInfo->bCurShortGI20MHz = ((pHTInfo->bRegShortGI20MHz) ?
> > > -				    ((pPeerHTCap->ShortGI20Mhz == 1) ?
> > > -				    true : false) : false);
> > > +				    (pPeerHTCap->ShortGI20Mhz == 1) : false);
> > >  	pHTInfo->bCurShortGI40MHz = ((pHTInfo->bRegShortGI40MHz) ?
> > > -				     ((pPeerHTCap->ShortGI40Mhz == 1) ?
> > > -				     true : false) : false);
> > > +				     (pPeerHTCap->ShortGI40Mhz == 1) : false);
> >
> > Is this?  Kernel code is meant to be read by humans first, and the
> > compiler second.  None of this will be any different if you write out a
> > if () statement.  Please just do that instead.
> 
> Wouldn't && or || be nicer if possible?  If the == 1 are actually testing
> booleans, it could be quite concise.

Yes, maybe, but never ? : if at all possible (as function arguments it
makes sense.)

thanks,

greg k-h