Re: [PATCH] staging: rtl8723bs: Remove redundant else branches.

Alison Schofield
2022-03-29T19:58:51Z

Hi Sevinj,

You can do this to make sure your commit msg 'looks' similar to
others in the file:

$git log --pretty=oneline --abbrev-commit

d6ad258ae15d staging: rtl8723bs: Remove redundant else branches.
24e65aac9457 staging: rtl8723bs: remove rf type branching (fourth patch)
167fc30e8e51 staging: rtl8723bs: remove unused macros
d7361874468f staging: rtl8723bs: fix camel case in struct wlan_bcn_info
6994aa430368 staging: rtl8723bs: fix camel case in struct ndis_802_11_ssid

That tells me the 'Remove' should not start with an uppercase and
there should be no period at the end of line. Note that I look at this
per patch, because different drivers and subsystems have different
conventions. The point is to try to discern the prevailing style,
and follow it.

Please check the get_maintainers instruction in the first patch tutorial.
I only got this:

$ git show HEAD | perl scripts/get_maintainer.pl --separator , --nokeywords --nogit --nogit-fallback --norolestats
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,linux-staging@lists.linux.dev,linux-kernel@vger.kernel.org

On Tue, Mar 29, 2022 at 07:09:04AM -0700, Sevinj Aghayeva wrote:

Please state the 'why' of this patch. ie something like:
Adhere to Linux kernel coding style.

> This patch fixes the following checkpatch.pl warning:
The phrases 'This patch' is frowned upon as unecessarily wordy.
Perhaps simply:
Reported by checkpatch:

You have another patch in flight with similar things to update.
Please post a v2 of that one that addresses the feedback given
here.

I thought both patches had good scope in that they addressed multiple
instances of the same issue in a single file.

Thanks!
Alison

>
> WARNING: else is not generally useful after a break or return
>
> Signed-off-by: Sevinj Aghayeva <sevinj.aghayeva@gmail.com>
> ---
> .../staging/rtl8723bs/core/rtw_ieee80211.c | 32 ++++++++-----------
> 1 file changed, 13 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> index b449be537376..27de086903e2 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> @@ -94,16 +94,14 @@ bool rtw_is_cckratesonly_included(u8 *rate)
>
> int rtw_check_network_type(unsigned char *rate, int ratelen, int channel)
> {
> - if (channel > 14) {
> + if (channel > 14)
> return WIRELESS_INVALID;
> - } else { /* could be pure B, pure G, or B/G */
> - if (rtw_is_cckratesonly_included(rate))
> - return WIRELESS_11B;
> - else if (rtw_is_cckrates_included(rate))
> - return WIRELESS_11BG;
> - else
> - return WIRELESS_11G;
> - }
> + /* could be pure B, pure G, or B/G */
> + if (rtw_is_cckratesonly_included(rate))
> + return WIRELESS_11B;
> + if (rtw_is_cckrates_included(rate))
> + return WIRELESS_11BG;
> + return WIRELESS_11G;
> }
>
> u8 *rtw_set_fixed_ie(unsigned char *pbuf, unsigned int len, unsigned char *source,
> @@ -151,11 +149,10 @@ u8 *rtw_get_ie(u8 *pbuf, signed int index, signed int *len, signed int limit)
> if (*p == index) {
> *len = *(p + 1);
> return p;
> - } else {
> - tmp = *(p + 1);
> - p += (tmp + 2);
> - i += (tmp + 2);
> }
> + tmp = *(p + 1);
> + p += (tmp + 2);
> + i += (tmp + 2);
> if (i >= limit)
> break;
> }
> @@ -199,9 +196,8 @@ u8 *rtw_get_ie_ex(u8 *in_ie, uint in_len, u8 eid, u8 *oui, u8 oui_len, u8 *ie, u
> *ielen = in_ie[cnt+1]+2;
>
> break;
> - } else {
> - cnt += in_ie[cnt+1]+2; /* goto next */
> }
> + cnt += in_ie[cnt+1]+2; /* goto next */
> }
>
> return target_ie;
> @@ -697,9 +693,8 @@ u8 *rtw_get_wps_ie(u8 *in_ie, uint in_len, u8 *wps_ie, uint *wps_ielen)
> cnt += in_ie[cnt+1]+2;
>
> break;
> - } else {
> - cnt += in_ie[cnt+1]+2; /* goto next */
> }
> + cnt += in_ie[cnt+1]+2; /* goto next */
> }
>
> return wpsie_ptr;
> @@ -748,9 +743,8 @@ u8 *rtw_get_wps_attr(u8 *wps_ie, uint wps_ielen, u16 target_attr_id, u8 *buf_att
> *len_attr = attr_len;
>
> break;
> - } else {
> - attr_ptr += attr_len; /* goto next */
> }
> + attr_ptr += attr_len; /* goto next */
> }
>
> return target_attr_ptr;
> --
> 2.25.1
>
>