* [PATCH] Staging: rtl8192e: Remove ternary operator
@ 2016-10-08 18:28 Bhumika Goyal
2016-10-09 6:00 ` [Outreachy kernel] " Julia Lawall
2016-10-09 14:26 ` Greg KH
0 siblings, 2 replies; 8+ messages in thread
From: Bhumika Goyal @ 2016-10-08 18:28 UTC (permalink / raw)
To: outreachy-kernel, gregkh; +Cc: Bhumika Goyal
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);
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);
pHTInfo->bCurSuppCCK = ((pHTInfo->bRegSuppCCK) ?
- ((pPeerHTCap->DssCCk == 1) ? true :
- false) : false);
+ (pPeerHTCap->DssCCk == 1) : false);
pHTInfo->bCurrent_AMSDU_Support = pHTInfo->bAMSDU_Support;
diff --git a/drivers/staging/rtl8192e/rtllib.h b/drivers/staging/rtl8192e/rtllib.h
index b895a53..f2c28f3 100644
--- a/drivers/staging/rtl8192e/rtllib.h
+++ b/drivers/staging/rtl8192e/rtllib.h
@@ -464,7 +464,7 @@ struct ieee_param {
#define RTLLIB_QCTL_TID 0x000F
#define FC_QOS_BIT BIT7
-#define IsDataFrame(pdu) (((pdu[0] & 0x0C) == 0x08) ? true : false)
+#define IsDataFrame(pdu) ((pdu[0] & 0x0C) == 0x08)
#define IsLegacyDataFrame(pdu) (IsDataFrame(pdu) && (!(pdu[0]&FC_QOS_BIT)))
#define IsQoSDataFrame(pframe) \
((*(u16 *)pframe&(RTLLIB_STYPE_QOS_DATA|RTLLIB_FTYPE_DATA)) == \
--
1.9.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Outreachy kernel] [PATCH] Staging: rtl8192e: Remove ternary operator
2016-10-08 18:28 [PATCH] Staging: rtl8192e: Remove ternary operator Bhumika Goyal
@ 2016-10-09 6:00 ` Julia Lawall
2016-10-09 6:38 ` Bhumika Goyal
2016-10-09 14:26 ` Greg KH
1 sibling, 1 reply; 8+ messages in thread
From: Julia Lawall @ 2016-10-09 6:00 UTC (permalink / raw)
To: Bhumika Goyal; +Cc: outreachy-kernel, gregkh
On Sat, 8 Oct 2016, 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);
>
> 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);
>
> pHTInfo->bCurSuppCCK = ((pHTInfo->bRegSuppCCK) ?
> - ((pPeerHTCap->DssCCk == 1) ? true :
> - false) : false);
> + (pPeerHTCap->DssCCk == 1) : false);
Even with the simplification, these look like a mess. Could && and || be
used here?
julia
>
>
> pHTInfo->bCurrent_AMSDU_Support = pHTInfo->bAMSDU_Support;
> diff --git a/drivers/staging/rtl8192e/rtllib.h b/drivers/staging/rtl8192e/rtllib.h
> index b895a53..f2c28f3 100644
> --- a/drivers/staging/rtl8192e/rtllib.h
> +++ b/drivers/staging/rtl8192e/rtllib.h
> @@ -464,7 +464,7 @@ struct ieee_param {
> #define RTLLIB_QCTL_TID 0x000F
>
> #define FC_QOS_BIT BIT7
> -#define IsDataFrame(pdu) (((pdu[0] & 0x0C) == 0x08) ? true : false)
> +#define IsDataFrame(pdu) ((pdu[0] & 0x0C) == 0x08)
> #define IsLegacyDataFrame(pdu) (IsDataFrame(pdu) && (!(pdu[0]&FC_QOS_BIT)))
> #define IsQoSDataFrame(pframe) \
> ((*(u16 *)pframe&(RTLLIB_STYPE_QOS_DATA|RTLLIB_FTYPE_DATA)) == \
> --
> 1.9.1
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/1475951297-10364-1-git-send-email-bhumirks%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Outreachy kernel] [PATCH] Staging: rtl8192e: Remove ternary operator
2016-10-09 6:00 ` [Outreachy kernel] " Julia Lawall
@ 2016-10-09 6:38 ` Bhumika Goyal
2016-10-09 10:51 ` Julia Lawall
0 siblings, 1 reply; 8+ messages in thread
From: Bhumika Goyal @ 2016-10-09 6:38 UTC (permalink / raw)
To: Julia Lawall; +Cc: outreachy-kernel, gregkh
On Sun, Oct 9, 2016 at 11:30 AM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>
>
> On Sat, 8 Oct 2016, 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);
>>
>> 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);
>>
>> pHTInfo->bCurSuppCCK = ((pHTInfo->bRegSuppCCK) ?
>> - ((pPeerHTCap->DssCCk == 1) ? true :
>> - false) : false);
>> + (pPeerHTCap->DssCCk == 1) : false);
>
> Even with the simplification, these look like a mess. Could && and || be
> used here?
>
Yes, I think these can be further reduced to something like:
pHTInfo->bCurSuppCCK= (pHTInfo->bRegSuppCCK && pPeerHTCap->DssCCk);
as the left hand side is true only if both the RHS values are true/1.
Same reduction could be done for pHTInfo->bCurShortGI40MHz and
pHTInfo->bCurShortGI20MHz.
Thanks,
Bhumika
> julia
>
>>
>>
>> pHTInfo->bCurrent_AMSDU_Support = pHTInfo->bAMSDU_Support;
>> diff --git a/drivers/staging/rtl8192e/rtllib.h b/drivers/staging/rtl8192e/rtllib.h
>> index b895a53..f2c28f3 100644
>> --- a/drivers/staging/rtl8192e/rtllib.h
>> +++ b/drivers/staging/rtl8192e/rtllib.h
>> @@ -464,7 +464,7 @@ struct ieee_param {
>> #define RTLLIB_QCTL_TID 0x000F
>>
>> #define FC_QOS_BIT BIT7
>> -#define IsDataFrame(pdu) (((pdu[0] & 0x0C) == 0x08) ? true : false)
>> +#define IsDataFrame(pdu) ((pdu[0] & 0x0C) == 0x08)
>> #define IsLegacyDataFrame(pdu) (IsDataFrame(pdu) && (!(pdu[0]&FC_QOS_BIT)))
>> #define IsQoSDataFrame(pframe) \
>> ((*(u16 *)pframe&(RTLLIB_STYPE_QOS_DATA|RTLLIB_FTYPE_DATA)) == \
>> --
>> 1.9.1
>>
>> --
>> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
>> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
>> To post to this group, send email to outreachy-kernel@googlegroups.com.
>> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/1475951297-10364-1-git-send-email-bhumirks%40gmail.com.
>> For more options, visit https://groups.google.com/d/optout.
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Outreachy kernel] [PATCH] Staging: rtl8192e: Remove ternary operator
2016-10-09 6:38 ` Bhumika Goyal
@ 2016-10-09 10:51 ` Julia Lawall
0 siblings, 0 replies; 8+ messages in thread
From: Julia Lawall @ 2016-10-09 10:51 UTC (permalink / raw)
To: Bhumika Goyal; +Cc: Julia Lawall, outreachy-kernel, gregkh
On Sun, 9 Oct 2016, Bhumika Goyal wrote:
> On Sun, Oct 9, 2016 at 11:30 AM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> >
> >
> > On Sat, 8 Oct 2016, 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);
> >>
> >> 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);
> >>
> >> pHTInfo->bCurSuppCCK = ((pHTInfo->bRegSuppCCK) ?
> >> - ((pPeerHTCap->DssCCk == 1) ? true :
> >> - false) : false);
> >> + (pPeerHTCap->DssCCk == 1) : false);
> >
> > Even with the simplification, these look like a mess. Could && and || be
> > used here?
> >
> Yes, I think these can be further reduced to something like:
> pHTInfo->bCurSuppCCK= (pHTInfo->bRegSuppCCK && pPeerHTCap->DssCCk);
> as the left hand side is true only if both the RHS values are true/1.
> Same reduction could be done for pHTInfo->bCurShortGI40MHz and
> pHTInfo->bCurShortGI20MHz.
Is DssCCk boolean? If not you may need to keep the == 1.
julia
>
> Thanks,
> Bhumika
>
> > julia
> >
> >>
> >>
> >> pHTInfo->bCurrent_AMSDU_Support = pHTInfo->bAMSDU_Support;
> >> diff --git a/drivers/staging/rtl8192e/rtllib.h b/drivers/staging/rtl8192e/rtllib.h
> >> index b895a53..f2c28f3 100644
> >> --- a/drivers/staging/rtl8192e/rtllib.h
> >> +++ b/drivers/staging/rtl8192e/rtllib.h
> >> @@ -464,7 +464,7 @@ struct ieee_param {
> >> #define RTLLIB_QCTL_TID 0x000F
> >>
> >> #define FC_QOS_BIT BIT7
> >> -#define IsDataFrame(pdu) (((pdu[0] & 0x0C) == 0x08) ? true : false)
> >> +#define IsDataFrame(pdu) ((pdu[0] & 0x0C) == 0x08)
> >> #define IsLegacyDataFrame(pdu) (IsDataFrame(pdu) && (!(pdu[0]&FC_QOS_BIT)))
> >> #define IsQoSDataFrame(pframe) \
> >> ((*(u16 *)pframe&(RTLLIB_STYPE_QOS_DATA|RTLLIB_FTYPE_DATA)) == \
> >> --
> >> 1.9.1
> >>
> >> --
> >> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> >> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> >> To post to this group, send email to outreachy-kernel@googlegroups.com.
> >> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/1475951297-10364-1-git-send-email-bhumirks%40gmail.com.
> >> For more options, visit https://groups.google.com/d/optout.
> >>
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/CAOH%2B1jGUWFyq5gJJq%3D1MhrjZjXyW0zeF4%2Bm8g%2BjvVPurerLg0w%40mail.gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Staging: rtl8192e: Remove ternary operator
2016-10-08 18:28 [PATCH] Staging: rtl8192e: Remove ternary operator Bhumika Goyal
2016-10-09 6:00 ` [Outreachy kernel] " Julia Lawall
@ 2016-10-09 14:26 ` Greg KH
2016-10-09 14:36 ` [Outreachy kernel] " Julia Lawall
1 sibling, 1 reply; 8+ messages in thread
From: Greg KH @ 2016-10-09 14:26 UTC (permalink / raw)
To: Bhumika Goyal; +Cc: outreachy-kernel
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.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH] Staging: rtl8192e: Remove ternary operator
2016-10-09 14:26 ` Greg KH
@ 2016-10-09 14:36 ` Julia Lawall
2016-10-09 14:46 ` Greg KH
0 siblings, 1 reply; 8+ messages in thread
From: Julia Lawall @ 2016-10-09 14:36 UTC (permalink / raw)
To: Greg KH; +Cc: Bhumika Goyal, outreachy-kernel
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.
julia
>
> thanks,
>
> greg k-h
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20161009142656.GA27065%40kroah.com.
> For more options, visit https://groups.google.com/d/optout.
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH] Staging: rtl8192e: Remove ternary operator
2016-10-09 14:36 ` [Outreachy kernel] " Julia Lawall
@ 2016-10-09 14:46 ` Greg KH
2016-10-09 16:21 ` Bhumika Goyal
0 siblings, 1 reply; 8+ messages in thread
From: Greg KH @ 2016-10-09 14:46 UTC (permalink / raw)
To: Julia Lawall; +Cc: Bhumika Goyal, outreachy-kernel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH] Staging: rtl8192e: Remove ternary operator
2016-10-09 14:46 ` Greg KH
@ 2016-10-09 16:21 ` Bhumika Goyal
0 siblings, 0 replies; 8+ messages in thread
From: Bhumika Goyal @ 2016-10-09 16:21 UTC (permalink / raw)
To: Greg KH; +Cc: Julia Lawall, outreachy-kernel
On Sun, Oct 9, 2016 at 8:16 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> 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.)
>
Okay, I will convert all these ?: operators into if() statements.
Thanks for the feedback Julia and Greg.
Thanks,
Bhumika
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-10-09 16:21 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-08 18:28 [PATCH] Staging: rtl8192e: Remove ternary operator Bhumika Goyal
2016-10-09 6:00 ` [Outreachy kernel] " Julia Lawall
2016-10-09 6:38 ` Bhumika Goyal
2016-10-09 10:51 ` Julia Lawall
2016-10-09 14:26 ` Greg KH
2016-10-09 14:36 ` [Outreachy kernel] " Julia Lawall
2016-10-09 14:46 ` Greg KH
2016-10-09 16:21 ` Bhumika Goyal
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.