* [RESEND PATCH] staging: rtl8188eu: Fix else after return WARNING (checkpatch) @ 2020-09-13 6:49 Sohom Datta 2020-09-13 6:52 ` Greg Kroah-Hartman 2020-09-14 14:57 ` Dan Carpenter 0 siblings, 2 replies; 6+ messages in thread From: Sohom Datta @ 2020-09-13 6:49 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Larry Finger, devel, linux-kernel From 4c8c8f3ff7f4d711daea4ac3bb987fcecc7ef1ed Mon Sep 17 00:00:00 2001 From: Sohom <sohom.datta@learner.manipal.edu> Date: Sat, 12 Sep 2020 18:04:56 +0530 Subject: [RESEND PATCH] staging: rtl8188eu: Fix else after return WARNING (checkpatch) Fixed: WARNING: else is not generally useful after a break or return 1636: FILE: ./rtw_recv.c:1636: + return false; + else Separated the return statement into a separate block since it doesn't seem to depend on the SN_LESS explicity being false. Signed-off-by: Sohom <sohom.datta@learner.manipal.edu> --- drivers/staging/rtl8188eu/core/rtw_recv.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/staging/rtl8188eu/core/rtw_recv.c b/drivers/staging/rtl8188eu/core/rtw_recv.c index 5fe7a0458dd2..5e81134ffb6d 100644 --- a/drivers/staging/rtl8188eu/core/rtw_recv.c +++ b/drivers/staging/rtl8188eu/core/rtw_recv.c @@ -1629,10 +1629,11 @@ static int enqueue_reorder_recvframe(struct recv_reorder_ctrl *preorder_ctrl, hdr = list_entry(plist, struct recv_frame, list); pnextattrib = &hdr->attrib; + if (SN_EQUAL(pnextattrib->seq_num, pattrib->seq_num)) + return false; + if (SN_LESS(pnextattrib->seq_num, pattrib->seq_num)) plist = plist->next; - else if (SN_EQUAL(pnextattrib->seq_num, pattrib->seq_num)) - return false; else break; } -- 2.28.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RESEND PATCH] staging: rtl8188eu: Fix else after return WARNING (checkpatch) 2020-09-13 6:49 [RESEND PATCH] staging: rtl8188eu: Fix else after return WARNING (checkpatch) Sohom Datta @ 2020-09-13 6:52 ` Greg Kroah-Hartman 2020-09-14 14:57 ` Dan Carpenter 1 sibling, 0 replies; 6+ messages in thread From: Greg Kroah-Hartman @ 2020-09-13 6:52 UTC (permalink / raw) To: Sohom Datta; +Cc: Larry Finger, devel, linux-kernel On Sun, Sep 13, 2020 at 12:19:50PM +0530, Sohom Datta wrote: > >From 4c8c8f3ff7f4d711daea4ac3bb987fcecc7ef1ed Mon Sep 17 00:00:00 2001 > From: Sohom <sohom.datta@learner.manipal.edu> > Date: Sat, 12 Sep 2020 18:04:56 +0530 > Subject: [RESEND PATCH] staging: rtl8188eu: Fix else after return WARNING > (checkpatch) Why is this in the body of your email? Just use 'git send-email' to send the patch, no need to include it in the body itself, right? thanks, greg k-h ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND PATCH] staging: rtl8188eu: Fix else after return WARNING (checkpatch) 2020-09-13 6:49 [RESEND PATCH] staging: rtl8188eu: Fix else after return WARNING (checkpatch) Sohom Datta 2020-09-13 6:52 ` Greg Kroah-Hartman @ 2020-09-14 14:57 ` Dan Carpenter 2020-09-14 16:42 ` Joe Perches 1 sibling, 1 reply; 6+ messages in thread From: Dan Carpenter @ 2020-09-14 14:57 UTC (permalink / raw) To: Sohom Datta; +Cc: Greg Kroah-Hartman, devel, linux-kernel, Larry Finger On Sun, Sep 13, 2020 at 12:19:50PM +0530, Sohom Datta wrote: > >From 4c8c8f3ff7f4d711daea4ac3bb987fcecc7ef1ed Mon Sep 17 00:00:00 2001 > From: Sohom <sohom.datta@learner.manipal.edu> > Date: Sat, 12 Sep 2020 18:04:56 +0530 > Subject: [RESEND PATCH] staging: rtl8188eu: Fix else after return WARNING > (checkpatch) > > Fixed: > WARNING: else is not generally useful after a break or return > 1636: FILE: ./rtw_recv.c:1636: > + return false; > + else > > Separated the return statement into a separate block since > it doesn't seem to depend on the SN_LESS explicity being false. > > Signed-off-by: Sohom <sohom.datta@learner.manipal.edu> > --- > drivers/staging/rtl8188eu/core/rtw_recv.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/rtl8188eu/core/rtw_recv.c b/drivers/staging/rtl8188eu/core/rtw_recv.c > index 5fe7a0458dd2..5e81134ffb6d 100644 > --- a/drivers/staging/rtl8188eu/core/rtw_recv.c > +++ b/drivers/staging/rtl8188eu/core/rtw_recv.c > @@ -1629,10 +1629,11 @@ static int enqueue_reorder_recvframe(struct recv_reorder_ctrl *preorder_ctrl, > hdr = list_entry(plist, struct recv_frame, list); > pnextattrib = &hdr->attrib; > > + if (SN_EQUAL(pnextattrib->seq_num, pattrib->seq_num)) > + return false; > + > if (SN_LESS(pnextattrib->seq_num, pattrib->seq_num)) > plist = plist->next; > - else if (SN_EQUAL(pnextattrib->seq_num, pattrib->seq_num)) > - return false; > else > break; > } Checkpatch is just wrong here. Ignore it when it's wrong. regards, dan carpenter ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND PATCH] staging: rtl8188eu: Fix else after return WARNING (checkpatch) 2020-09-14 14:57 ` Dan Carpenter @ 2020-09-14 16:42 ` Joe Perches 2020-09-15 11:20 ` Dan Carpenter 0 siblings, 1 reply; 6+ messages in thread From: Joe Perches @ 2020-09-14 16:42 UTC (permalink / raw) To: Dan Carpenter, Sohom Datta Cc: Greg Kroah-Hartman, devel, linux-kernel, Larry Finger On Mon, 2020-09-14 at 17:57 +0300, Dan Carpenter wrote: > On Sun, Sep 13, 2020 at 12:19:50PM +0530, Sohom Datta wrote: > > > From 4c8c8f3ff7f4d711daea4ac3bb987fcecc7ef1ed Mon Sep 17 00:00:00 2001 > > From: Sohom <sohom.datta@learner.manipal.edu> > > Date: Sat, 12 Sep 2020 18:04:56 +0530 > > Subject: [RESEND PATCH] staging: rtl8188eu: Fix else after return WARNING > > (checkpatch) > > > > Fixed: > > WARNING: else is not generally useful after a break or return > > 1636: FILE: ./rtw_recv.c:1636: > > + return false; > > + else > > > > Separated the return statement into a separate block since > > it doesn't seem to depend on the SN_LESS explicity being false. > > > > Signed-off-by: Sohom <sohom.datta@learner.manipal.edu> > > --- > > drivers/staging/rtl8188eu/core/rtw_recv.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/staging/rtl8188eu/core/rtw_recv.c b/drivers/staging/rtl8188eu/core/rtw_recv.c > > index 5fe7a0458dd2..5e81134ffb6d 100644 > > --- a/drivers/staging/rtl8188eu/core/rtw_recv.c > > +++ b/drivers/staging/rtl8188eu/core/rtw_recv.c > > @@ -1629,10 +1629,11 @@ static int enqueue_reorder_recvframe(struct recv_reorder_ctrl *preorder_ctrl, > > hdr = list_entry(plist, struct recv_frame, list); > > pnextattrib = &hdr->attrib; > > > > + if (SN_EQUAL(pnextattrib->seq_num, pattrib->seq_num)) > > + return false; > > + > > if (SN_LESS(pnextattrib->seq_num, pattrib->seq_num)) > > plist = plist->next; > > - else if (SN_EQUAL(pnextattrib->seq_num, pattrib->seq_num)) > > - return false; > > else > > break; > > } > > Checkpatch is just wrong here. Ignore it when it's wrong. It's not "wrong" here. It's making a suggestion. Perhaps read the SN_EQUAL and SN_LESS macros. a and b are both u16's here. drivers/staging/rtl8188eu/include/rtw_recv.h:#define SN_LESS(a, b) (((a - b) & 0x800) != 0) drivers/staging/rtl8188eu/include/rtw_recv.h:#define SN_EQUAL(a, b) (a == b) Reordering works, perhaps it's just a question of whether it's the most likely result of the test. This is in a while loop. If the expected test is really the most likely that SN_LESS is true, then perhaps this loop could be something like: if (SN_LESS(pnextattrib->seq_num, pattrib->seq_num)) { plist = plist->next; continue; } if (SN_EQUAL(pnextattrib->seq_num, pattrib->seq_num)) return false; break; } The real question is whether or not that's more readable. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND PATCH] staging: rtl8188eu: Fix else after return WARNING (checkpatch) 2020-09-14 16:42 ` Joe Perches @ 2020-09-15 11:20 ` Dan Carpenter 2020-09-15 15:25 ` Joe Perches 0 siblings, 1 reply; 6+ messages in thread From: Dan Carpenter @ 2020-09-15 11:20 UTC (permalink / raw) To: Joe Perches, Sebastian Arriola Cc: Sohom Datta, devel, Greg Kroah-Hartman, linux-kernel, Larry Finger On Mon, Sep 14, 2020 at 09:42:49AM -0700, Joe Perches wrote: > On Mon, 2020-09-14 at 17:57 +0300, Dan Carpenter wrote: > > On Sun, Sep 13, 2020 at 12:19:50PM +0530, Sohom Datta wrote: > > > > From 4c8c8f3ff7f4d711daea4ac3bb987fcecc7ef1ed Mon Sep 17 00:00:00 2001 > > > From: Sohom <sohom.datta@learner.manipal.edu> > > > Date: Sat, 12 Sep 2020 18:04:56 +0530 > > > Subject: [RESEND PATCH] staging: rtl8188eu: Fix else after return WARNING > > > (checkpatch) > > > > > > Fixed: > > > WARNING: else is not generally useful after a break or return > > > 1636: FILE: ./rtw_recv.c:1636: > > > + return false; > > > + else > > > > > > Separated the return statement into a separate block since > > > it doesn't seem to depend on the SN_LESS explicity being false. > > > > > > Signed-off-by: Sohom <sohom.datta@learner.manipal.edu> > > > --- > > > drivers/staging/rtl8188eu/core/rtw_recv.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/staging/rtl8188eu/core/rtw_recv.c b/drivers/staging/rtl8188eu/core/rtw_recv.c > > > index 5fe7a0458dd2..5e81134ffb6d 100644 > > > --- a/drivers/staging/rtl8188eu/core/rtw_recv.c > > > +++ b/drivers/staging/rtl8188eu/core/rtw_recv.c > > > @@ -1629,10 +1629,11 @@ static int enqueue_reorder_recvframe(struct recv_reorder_ctrl *preorder_ctrl, > > > hdr = list_entry(plist, struct recv_frame, list); > > > pnextattrib = &hdr->attrib; > > > > > > + if (SN_EQUAL(pnextattrib->seq_num, pattrib->seq_num)) > > > + return false; > > > + > > > if (SN_LESS(pnextattrib->seq_num, pattrib->seq_num)) > > > plist = plist->next; > > > - else if (SN_EQUAL(pnextattrib->seq_num, pattrib->seq_num)) > > > - return false; > > > else > > > break; > > > } > > > > Checkpatch is just wrong here. Ignore it when it's wrong. > > It's not "wrong" here. It's making a suggestion. > > Perhaps read the SN_EQUAL and SN_LESS macros. > > a and b are both u16's here. > > drivers/staging/rtl8188eu/include/rtw_recv.h:#define SN_LESS(a, b) (((a - b) & 0x800) != 0) > drivers/staging/rtl8188eu/include/rtw_recv.h:#define SN_EQUAL(a, b) (a == b) > > Reordering works, perhaps it's just a question of > whether it's the most likely result of the test. > > This is in a while loop. > > If the expected test is really the most likely that > SN_LESS is true, then perhaps this loop could be > something like: > > if (SN_LESS(pnextattrib->seq_num, pattrib->seq_num)) { > plist = plist->next; > continue; > } > if (SN_EQUAL(pnextattrib->seq_num, pattrib->seq_num)) > return false; > break; > } > > The real question is whether or not that's more readable. > It's not clear to me that any of these are more readable than the other. I see that someone broke the staging/rtl8712 version of this driver in June. See commit 98fe05e21a6e ("staging: rtl8712: Remove unnecesary else after return statement."). That patch went through LKML instead of going through the driver-devel list... :/ regards, dan carpenter ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND PATCH] staging: rtl8188eu: Fix else after return WARNING (checkpatch) 2020-09-15 11:20 ` Dan Carpenter @ 2020-09-15 15:25 ` Joe Perches 0 siblings, 0 replies; 6+ messages in thread From: Joe Perches @ 2020-09-15 15:25 UTC (permalink / raw) To: Dan Carpenter, Sebastian Arriola Cc: Sohom Datta, devel, Greg Kroah-Hartman, linux-kernel, Larry Finger On Tue, 2020-09-15 at 14:20 +0300, Dan Carpenter wrote: > On Mon, Sep 14, 2020 at 09:42:49AM -0700, Joe Perches wrote: > > On Mon, 2020-09-14 at 17:57 +0300, Dan Carpenter wrote: > > > On Sun, Sep 13, 2020 at 12:19:50PM +0530, Sohom Datta wrote: > > > > > From 4c8c8f3ff7f4d711daea4ac3bb987fcecc7ef1ed Mon Sep 17 00:00:00 2001 > > > > From: Sohom <sohom.datta@learner.manipal.edu> > > > > Date: Sat, 12 Sep 2020 18:04:56 +0530 > > > > Subject: [RESEND PATCH] staging: rtl8188eu: Fix else after return WARNING > > > > (checkpatch) > > > > > > > > Fixed: > > > > WARNING: else is not generally useful after a break or return > > > > 1636: FILE: ./rtw_recv.c:1636: > > > > + return false; > > > > + else > > > > > > > > Separated the return statement into a separate block since > > > > it doesn't seem to depend on the SN_LESS explicity being false. > > > > > > > > Signed-off-by: Sohom <sohom.datta@learner.manipal.edu> > > > > --- > > > > drivers/staging/rtl8188eu/core/rtw_recv.c | 5 +++-- > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/staging/rtl8188eu/core/rtw_recv.c b/drivers/staging/rtl8188eu/core/rtw_recv.c > > > > index 5fe7a0458dd2..5e81134ffb6d 100644 > > > > --- a/drivers/staging/rtl8188eu/core/rtw_recv.c > > > > +++ b/drivers/staging/rtl8188eu/core/rtw_recv.c > > > > @@ -1629,10 +1629,11 @@ static int enqueue_reorder_recvframe(struct recv_reorder_ctrl *preorder_ctrl, > > > > hdr = list_entry(plist, struct recv_frame, list); > > > > pnextattrib = &hdr->attrib; > > > > > > > > + if (SN_EQUAL(pnextattrib->seq_num, pattrib->seq_num)) > > > > + return false; > > > > + > > > > if (SN_LESS(pnextattrib->seq_num, pattrib->seq_num)) > > > > plist = plist->next; > > > > - else if (SN_EQUAL(pnextattrib->seq_num, pattrib->seq_num)) > > > > - return false; > > > > else > > > > break; > > > > } > > > > > > Checkpatch is just wrong here. Ignore it when it's wrong. > > > > It's not "wrong" here. It's making a suggestion. > > > > Perhaps read the SN_EQUAL and SN_LESS macros. > > > > a and b are both u16's here. > > > > drivers/staging/rtl8188eu/include/rtw_recv.h:#define SN_LESS(a, b) (((a - b) & 0x800) != 0) > > drivers/staging/rtl8188eu/include/rtw_recv.h:#define SN_EQUAL(a, b) (a == b) > > > > Reordering works, perhaps it's just a question of > > whether it's the most likely result of the test. > > > > This is in a while loop. > > > > If the expected test is really the most likely that > > SN_LESS is true, then perhaps this loop could be > > something like: > > > > if (SN_LESS(pnextattrib->seq_num, pattrib->seq_num)) { > > plist = plist->next; > > continue; > > } > > if (SN_EQUAL(pnextattrib->seq_num, pattrib->seq_num)) > > return false; > > break; > > } > > > > The real question is whether or not that's more readable. > > > > It's not clear to me that any of these are more readable than the other. > > I see that someone broke the staging/rtl8712 version of this driver in > June. See commit 98fe05e21a6e ("staging: rtl8712: Remove unnecesary > else after return statement."). That patch went through LKML instead of > going through the driver-devel list... :/ That's sad. Then another question is whether either is more prone to unintentional breakage by novice code readers being guided by brainless scripts... A bit of a pity as the original intent of the checkpatch test was somewhat useful. Maybe it's outlived its value though when used with -f files. There aren't many of these left in the kernel. Maybe it should be changed to work only on patches. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-09-15 17:22 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-09-13 6:49 [RESEND PATCH] staging: rtl8188eu: Fix else after return WARNING (checkpatch) Sohom Datta 2020-09-13 6:52 ` Greg Kroah-Hartman 2020-09-14 14:57 ` Dan Carpenter 2020-09-14 16:42 ` Joe Perches 2020-09-15 11:20 ` Dan Carpenter 2020-09-15 15:25 ` Joe Perches
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox