From: Hans de Goede <hdegoede@redhat.com>
To: kushal kothari <kushalkothari285@gmail.com>,
Mike Rapoport <mike.rapoport@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
fabioaiuto83@gmail.com, ross.schm.dev@gmail.com,
marcocesati@gmail.com,
"Fabio M. De Francesco" <fmdefrancesco@gmail.com>,
linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org,
outreachy-kernel@googlegroups.com, kushalkothari2850@gmail.com
Subject: Re: [PATCH v2] staging: rtl8723bs: core: Refactor nested if-else
Date: Tue, 26 Oct 2021 17:15:13 +0200 [thread overview]
Message-ID: <eb7db8fa-0557-425f-b08d-1d55ac07610a@redhat.com> (raw)
In-Reply-To: <CALtHPQuKDa23Ui4wBVed=7FOkf6u+-MjL1VU-4CD6hXSjmsq6g@mail.gmail.com>
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@gmail.com <mailto:mike.rapoport@gmail.com>> 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@gmail.com <mailto:kushalkothari285@gmail.com>>
> > ---
> >
> > 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.
>
prev parent reply other threads:[~2021-10-26 15:15 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-26 13:42 [PATCH v2] staging: rtl8723bs: core: Refactor nested if-else Kushal Kothari
2021-10-26 13:54 ` [Outreachy kernel] " Praveen Kumar
[not found] ` <CALtHPQsh=0LyM3K9dn8d0X+n4gbx2TApF6k_WhpypHXGj6Hj-w@mail.gmail.com>
2021-10-26 17:36 ` Praveen Kumar
2021-10-26 14:19 ` Mike Rapoport
[not found] ` <CALtHPQuKDa23Ui4wBVed=7FOkf6u+-MjL1VU-4CD6hXSjmsq6g@mail.gmail.com>
2021-10-26 15:15 ` Hans de Goede [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=eb7db8fa-0557-425f-b08d-1d55ac07610a@redhat.com \
--to=hdegoede@redhat.com \
--cc=fabioaiuto83@gmail.com \
--cc=fmdefrancesco@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=kushalkothari2850@gmail.com \
--cc=kushalkothari285@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-staging@lists.linux.dev \
--cc=marcocesati@gmail.com \
--cc=mike.rapoport@gmail.com \
--cc=outreachy-kernel@googlegroups.com \
--cc=ross.schm.dev@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox