Re: [PATCH v2] staging: rtl8723bs: core: Refactor nested if-else
From: Hans de Goede
Date: Tue Oct 26 2021 - 11:15:21 EST
Hi,
On 10/26/21 17:12, kushal kothari wrote:
> Yes!
> Also initializing variable pstat->has_legacy_ac = false; //false
> which is previously set to true before the loop so it can be removed too.
Note please do not make too many changes in a single patch.
It is best to do refactoring like this in small steps making
only 1 change at a time, that makes things a lot easier to
review / to verify that there are no functional changes.
Regards,
Hans
>
> On Tue, Oct 26, 2021 at 7:49 PM Mike Rapoport <mike.rapoport@xxxxxxxxx <mailto:mike.rapoport@xxxxxxxxx>> wrote:
>
> On Tue, Oct 26, 2021 at 07:12:53PM +0530, Kushal Kothari wrote:
> > Refactor nested if-else to avoid deep indentations. There is no change
> > in the logic of the new code, however, now it is simple because it gets
> > rid of five unnecessary else conditionals and it combines nested if into
> > single if-else-if. This refactor also leads to fix warning detected by
> > checkpatch.pl <http://checkpatch.pl>:
> > WARNING: Too many leading tabs - consider code refactoring
> >
> > Signed-off-by: Kushal Kothari <kushalkothari285@xxxxxxxxx <mailto:kushalkothari285@xxxxxxxxx>>
> > ---
> >
> > Changes in v2: Fix the bug of not handling properly the else logic
> > when p is not null in else-if. Also, reword the subject line and break
> > it up at 72 columns.
> >
> > drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 69 ++++++++-----------
> > 1 file changed, 29 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> > index 0f82f5031c43..267d853b1514 100644
> > --- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> > +++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> > @@ -1192,50 +1192,39 @@ unsigned int OnAssocReq(struct adapter *padapter, union recv_frame *precv_frame)
> > p = pframe + WLAN_HDR_A3_LEN + ie_offset; ie_len = 0;
> > for (;;) {
> > p = rtw_get_ie(p, WLAN_EID_VENDOR_SPECIFIC, &ie_len, pkt_len - WLAN_HDR_A3_LEN - ie_offset);
> > - if (p) {
> > - if (!memcmp(p+2, WMM_IE, 6)) {
> > -
> > - pstat->flags |= WLAN_STA_WME;
> > -
> > - pstat->qos_option = 1;
> > - pstat->qos_info = *(p+8);
> > -
> > - pstat->max_sp_len = (pstat->qos_info>>5)&0x3;
> > -
> > - if ((pstat->qos_info&0xf) != 0xf)
> > - pstat->has_legacy_ac = true;
> > - else
> > - pstat->has_legacy_ac = false;
> > -
> > - if (pstat->qos_info&0xf) {
> > - if (pstat->qos_info&BIT(0))
> > - pstat->uapsd_vo = BIT(0)|BIT(1);
> > - else
> > - pstat->uapsd_vo = 0;
> > -
> > - if (pstat->qos_info&BIT(1))
> > - pstat->uapsd_vi = BIT(0)|BIT(1);
> > - else
> > - pstat->uapsd_vi = 0;
> > -
> > - if (pstat->qos_info&BIT(2))
> > - pstat->uapsd_bk = BIT(0)|BIT(1);
> > - else
> > - pstat->uapsd_bk = 0;
> > -
> > - if (pstat->qos_info&BIT(3))
> > - pstat->uapsd_be = BIT(0)|BIT(1);
> > - else
> > - pstat->uapsd_be = 0;
> > -
> > - }
> > -
> > - break;
> > + if (p && memcmp(p+2, WMM_IE, 6)) {
> > + p = p + ie_len + 2;
> > + } else if (p && !memcmp(p+2, WMM_IE, 6)) {
> > + pstat->flags |= WLAN_STA_WME;
> > + pstat->qos_option = 1;
> > + pstat->qos_info = *(p+8);
> > + pstat->max_sp_len = (pstat->qos_info>>5)&0x3;
> > +
> > + pstat->has_legacy_ac = false;
> > + if ((pstat->qos_info&0xf) != 0xf)
> > + pstat->has_legacy_ac = true;
> > +
> > + if (pstat->qos_info&0xf) {
> > + pstat->uapsd_vo = 0;
>
> This variable and other variables below are set to 0 just before the loop,
> so the initialization here can be removed.
>
> > + if (pstat->qos_info&BIT(0))
> > + pstat->uapsd_vo = BIT(0)|BIT(1);
> > +
> > + pstat->uapsd_vi = 0;
> > + if (pstat->qos_info&BIT(1))
> > + pstat->uapsd_vi = BIT(0)|BIT(1);
> > +
> > + pstat->uapsd_bk = 0;
> > + if (pstat->qos_info&BIT(2))
> > + pstat->uapsd_bk = BIT(0)|BIT(1);
> > +
> > + pstat->uapsd_be = 0;
> > + if (pstat->qos_info&BIT(3))
> > + pstat->uapsd_be = BIT(0)|BIT(1);
> > }
> > + break;
> > } else {
> > break;
> > }
> > - p = p + ie_len + 2;
> > }
> > }
> >
> > --
> > 2.25.1
> >
>
> --
> Sincerely yours,
> Mike.
>