| Message ID | 20160129172908.GA14077@Karyakshetra (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Johannes Berg |
| Headers | show |
Bhaktipriya Shridhar <bhaktipriya96@gmail.com> writes: > This patch fixes checkpatch.pl warning in rtw_mlme_ext.c file. > WARNING: void function return statements are not generally useful > > Signed-off-by: Bhaktipriya Shridhar <bhaktipriya96@gmail.com> > --- > drivers/staging/rtl8723au/core/rtw_mlme_ext.c | 10 ---------- > 1 file changed, 10 deletions(-) > > diff --git a/drivers/staging/rtl8723au/core/rtw_mlme_ext.c b/drivers/staging/rtl8723au/core/rtw_mlme_ext.c > index d28f29a..e8a16b9 100644 > --- a/drivers/staging/rtl8723au/core/rtw_mlme_ext.c > +++ b/drivers/staging/rtl8723au/core/rtw_mlme_ext.c > @@ -2657,7 +2657,6 @@ static void issue_probersp(struct rtw_adapter *padapter, unsigned char *da) > > dump_mgntframe23a(padapter, pmgntframe); > > - return; > } If you insist on pushing this rather unncessary change, please do it properly, and remove the blank line before the return statement as well. Jes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bhaktipriya, On Sat, Jan 30, 2016 at 5:00 AM, Jes Sorensen <Jes.Sorensen@redhat.com> wrote: > Bhaktipriya Shridhar <bhaktipriya96@gmail.com> writes: >> This patch fixes checkpatch.pl warning in rtw_mlme_ext.c file. >> WARNING: void function return statements are not generally useful >> >> Signed-off-by: Bhaktipriya Shridhar <bhaktipriya96@gmail.com> >> --- >> drivers/staging/rtl8723au/core/rtw_mlme_ext.c | 10 ---------- >> 1 file changed, 10 deletions(-) >> >> diff --git a/drivers/staging/rtl8723au/core/rtw_mlme_ext.c b/drivers/staging/rtl8723au/core/rtw_mlme_ext.c >> index d28f29a..e8a16b9 100644 >> --- a/drivers/staging/rtl8723au/core/rtw_mlme_ext.c >> +++ b/drivers/staging/rtl8723au/core/rtw_mlme_ext.c >> @@ -2657,7 +2657,6 @@ static void issue_probersp(struct rtw_adapter *padapter, unsigned char *da) >> >> dump_mgntframe23a(padapter, pmgntframe); >> >> - return; >> } > > If you insist on pushing this rather unncessary change, please do it > properly, and remove the blank line before the return statement as well. As Jes said, you need to remove the blank lines before the returns too. checkpatch should have picked this up, you did run the patch through checkpatch before you sent it, right? Jes, I know you have strong feelings on coding style, but there are a lot of people out there who see deviations from the standard as bugs to be fixed, so stuff like this isn't going to stop until it matches the coding style document's spec. Thanks,
On Sat, 2016-01-30 at 10:17 +1100, Julian Calaby wrote: > On Sat, Jan 30, 2016 at 5:00 AM, Jes Sorensen <Jes.Sorensen@redhat.com> wrote: > > Bhaktipriya Shridhar <bhaktipriya96@gmail.com> writes: > > > This patch fixes checkpatch.pl warning in rtw_mlme_ext.c file. > > > WARNING: void function return statements are not generally useful [] > > > diff --git a/drivers/staging/rtl8723au/core/rtw_mlme_ext.c b/drivers/staging/rtl8723au/core/rtw_mlme_ext.c [] > > > @@ -2657,7 +2657,6 @@ static void issue_probersp(struct rtw_adapter *padapter, unsigned char *da) > > > > > > dump_mgntframe23a(padapter, pmgntframe); > > > > > > - return; > > > } > > > > If you insist on pushing this rather unncessary change, please do it > > properly, and remove the blank line before the return statement as well. > > As Jes said, you need to remove the blank lines before the returns > too. checkpatch should have picked this up, you did run the patch > through checkpatch before you sent it, right? checkpatch doesn't pick this up. If you'd like to make it do so, you're welcome to try but it's likely a bit more complicated than it appears. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Joe, On Sat, Jan 30, 2016 at 12:28 PM, Joe Perches <joe@perches.com> wrote: > On Sat, 2016-01-30 at 10:17 +1100, Julian Calaby wrote: >> On Sat, Jan 30, 2016 at 5:00 AM, Jes Sorensen <Jes.Sorensen@redhat.com> wrote: >> > Bhaktipriya Shridhar <bhaktipriya96@gmail.com> writes: >> > > This patch fixes checkpatch.pl warning in rtw_mlme_ext.c file. >> > > WARNING: void function return statements are not generally useful > [] >> > > diff --git a/drivers/staging/rtl8723au/core/rtw_mlme_ext.c b/drivers/staging/rtl8723au/core/rtw_mlme_ext.c > [] >> > > @@ -2657,7 +2657,6 @@ static void issue_probersp(struct rtw_adapter *padapter, unsigned char *da) >> > > >> > > dump_mgntframe23a(padapter, pmgntframe); >> > > >> > > - return; >> > > } >> > >> > If you insist on pushing this rather unncessary change, please do it >> > properly, and remove the blank line before the return statement as well. >> >> As Jes said, you need to remove the blank lines before the returns >> too. checkpatch should have picked this up, you did run the patch >> through checkpatch before you sent it, right? > > checkpatch doesn't pick this up. > > If you'd like to make it do so, you're welcome to try > but it's likely a bit more complicated than it appears. I meant the extra blank lines, not the useless return statements. Thanks,
On Sat, 2016-01-30 at 14:09 +1100, Julian Calaby wrote: > Hi Joe, Hello Julian. > On Sat, Jan 30, 2016 at 12:28 PM, Joe Perches <joe@perches.com> > wrote: > > On Sat, 2016-01-30 at 10:17 +1100, Julian Calaby wrote: > > > On Sat, Jan 30, 2016 at 5:00 AM, Jes Sorensen <Jes.Sorensen@redha > > > t.com> wrote: > > > > Bhaktipriya Shridhar <bhaktipriya96@gmail.com> writes: > > > > > This patch fixes checkpatch.pl warning in rtw_mlme_ext.c > > > > > file. > > > > > WARNING: void function return statements are not generally > > > > > useful > > [] > > > > > diff --git a/drivers/staging/rtl8723au/core/rtw_mlme_ext.c > > > > > b/drivers/staging/rtl8723au/core/rtw_mlme_ext.c > > [] > > > > > @@ -2657,7 +2657,6 @@ static void issue_probersp(struct > > > > > rtw_adapter *padapter, unsigned char *da) > > > > > > > > > > dump_mgntframe23a(padapter, pmgntframe); > > > > > > > > > > - return; > > > > > } > > > > > > > > If you insist on pushing this rather unncessary change, please > > > > do it > > > > properly, and remove the blank line before the return statement > > > > as well. > > > > > > As Jes said, you need to remove the blank lines before the > > > returns > > > too. checkpatch should have picked this up, you did run the patch > > > through checkpatch before you sent it, right? > > > > checkpatch doesn't pick this up. > > > > If you'd like to make it do so, you're welcome to try > > but it's likely a bit more complicated than it appears. > > I meant the extra blank lines, not the useless return statements. I understood what you meant. It's relatively difficult to determine that a line removal patch causes a blank line to appear before a closing brace. You're welcome to extend checkpatch to find these things, but there are likely many additional patch types that need to be considered. Remember patches can add, modify and delete lines. cheers, Joe -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Thank you for your reply. I've just sent version 2 of the patch with the blank lines removed. I will be happy to extend checkpatch.pl. As suggested by you, I am trying to detect such blank lines in a line removal patch by checking if the line above the deleted line was a blank line and the line following the deleted line had a closing brace. Can you please guide me and let me know if I am headed in the right direction. Thanks, Bhaktipriya On Sat, Jan 30, 2016 at 8:48 AM, Joe Perches <joe@perches.com> wrote: > On Sat, 2016-01-30 at 14:09 +1100, Julian Calaby wrote: >> Hi Joe, > > Hello Julian. > >> On Sat, Jan 30, 2016 at 12:28 PM, Joe Perches <joe@perches.com> >> wrote: >> > On Sat, 2016-01-30 at 10:17 +1100, Julian Calaby wrote: >> > > On Sat, Jan 30, 2016 at 5:00 AM, Jes Sorensen <Jes.Sorensen@redha >> > > t.com> wrote: >> > > > Bhaktipriya Shridhar <bhaktipriya96@gmail.com> writes: >> > > > > This patch fixes checkpatch.pl warning in rtw_mlme_ext.c >> > > > > file. >> > > > > WARNING: void function return statements are not generally >> > > > > useful >> > [] >> > > > > diff --git a/drivers/staging/rtl8723au/core/rtw_mlme_ext.c >> > > > > b/drivers/staging/rtl8723au/core/rtw_mlme_ext.c >> > [] >> > > > > @@ -2657,7 +2657,6 @@ static void issue_probersp(struct >> > > > > rtw_adapter *padapter, unsigned char *da) >> > > > > >> > > > > dump_mgntframe23a(padapter, pmgntframe); >> > > > > >> > > > > - return; >> > > > > } >> > > > >> > > > If you insist on pushing this rather unncessary change, please >> > > > do it >> > > > properly, and remove the blank line before the return statement >> > > > as well. >> > > >> > > As Jes said, you need to remove the blank lines before the >> > > returns >> > > too. checkpatch should have picked this up, you did run the patch >> > > through checkpatch before you sent it, right? >> > >> > checkpatch doesn't pick this up. >> > >> > If you'd like to make it do so, you're welcome to try >> > but it's likely a bit more complicated than it appears. >> >> I meant the extra blank lines, not the useless return statements. > > I understood what you meant. > > It's relatively difficult to determine that a line removal > patch causes a blank line to appear before a closing brace. > > You're welcome to extend checkpatch to find these things, > but there are likely many additional patch types that need > to be considered. Remember patches can add, modify and > delete lines. > > cheers, Joe -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 2016-01-30 at 12:23 +0530, Bhakti Priya wrote: > I will be happy to extend checkpatch.pl. As suggested by you, I am > trying to detect such blank lines in a line removal patch by checking > if the line above the deleted line was a blank line and the line > following the deleted line had a closing brace. > Can you please guide me and let me know if I am headed in the right > direction. You have to track all the + and - lines that precede the deleted line. Good luck. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bhakti, On Sat, Jan 30, 2016 at 5:53 PM, Bhakti Priya <bhaktipriya96@gmail.com> wrote: > Hi, > > Thank you for your reply. I've just sent version 2 of the patch with > the blank lines removed. > I will be happy to extend checkpatch.pl. As suggested by you, I am > trying to detect such blank lines in a line removal patch by checking > if the line above the deleted line was a blank line and the line > following the deleted line had a closing brace. > Can you please guide me and let me know if I am headed in the right direction. As I understand it, the algorithm needs to work like this: 1. For each patch hunk: 2. Filter out all lines that match /^-/ 3. Remove the first character (" " or "+") 4. Normalise EOL characters: s/\r\n?/\n/ 5. Over the entire hunk, find any case that matches /({|\n)\s*\n\s*(\n|})/ where \s matches all space characters except \n. 6. Report the middle line the preceding regular expression matches to the user. I'm confident I can write it as a shell script, but I don't know enough Perl to add that test to checkpatch.pl Thanks,
On Sat, 2016-01-30 at 23:02 +1100, Julian Calaby wrote: > Hi Bhakti, > > On Sat, Jan 30, 2016 at 5:53 PM, Bhakti Priya wrote: > > Hi, > > > > Thank you for your reply. I've just sent version 2 of the patch with > > the blank lines removed. > > I will be happy to extend checkpatch.pl. As suggested by you, I am > > trying to detect such blank lines in a line removal patch by checking > > if the line above the deleted line was a blank line and the line > > following the deleted line had a closing brace. > > Can you please guide me and let me know if I am headed in the right direction. > > As I understand it, the algorithm needs to work like this: > 1. For each patch hunk: > 2. Filter out all lines that match /^-/ > 3. Remove the first character (" " or "+") > 4. Normalise EOL characters: s/\r\n?/\n/ > 5. Over the entire hunk, find any case that matches > /({|\n)\s*\n\s*(\n|})/ where \s matches all space characters except > \n. > 6. Report the middle line the preceding regular expression matches to the user. > > I'm confident I can write it as a shell script, but I don't know > enough Perl to add that test to checkpatch.pl That's basically what the $prevline variable in checkpatch does. Likely it's enough to check that. Perhaps Andy Whitcroft knows. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Julian Calaby <julian.calaby@gmail.com> writes: > Hi Bhaktipriya, > > On Sat, Jan 30, 2016 at 5:00 AM, Jes Sorensen <Jes.Sorensen@redhat.com> wrote: >> Bhaktipriya Shridhar <bhaktipriya96@gmail.com> writes: >> If you insist on pushing this rather unncessary change, please do it >> properly, and remove the blank line before the return statement as well. > > As Jes said, you need to remove the blank lines before the returns > too. checkpatch should have picked this up, you did run the patch > through checkpatch before you sent it, right? > > Jes, > > I know you have strong feelings on coding style, but there are a lot > of people out there who see deviations from the standard as bugs to be > fixed, so stuff like this isn't going to stop until it matches the > coding style document's spec. Julian, rtl8723au is pretty dead development wise, so I don't care too much. checkpatch is broken and has effectively turned into a policing tool for a few people who wish to apply their narrow view onto everyone else. I'll continue top reject broken patches to my code pushed out under those rules. Maybe it's time to introduce checkpatchconsideredharmful.com Jes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/staging/rtl8723au/core/rtw_mlme_ext.c b/drivers/staging/rtl8723au/core/rtw_mlme_ext.c index d28f29a..e8a16b9 100644 --- a/drivers/staging/rtl8723au/core/rtw_mlme_ext.c +++ b/drivers/staging/rtl8723au/core/rtw_mlme_ext.c @@ -2657,7 +2657,6 @@ static void issue_probersp(struct rtw_adapter *padapter, unsigned char *da) dump_mgntframe23a(padapter, pmgntframe); - return; } static int _issue_probereq(struct rtw_adapter *padapter, @@ -2958,7 +2957,6 @@ static void issue_auth(struct rtw_adapter *padapter, struct sta_info *psta, DBG_8723A("%s\n", __func__); dump_mgntframe23a(padapter, pmgntframe); - return; } #ifdef CONFIG_8723AU_AP_MODE @@ -3339,7 +3337,6 @@ exit: } else kfree(pmlmepriv->assoc_req); - return; } /* when wait_ack is true, this function should be called at process context */ @@ -4103,7 +4100,6 @@ static void rtw_site_survey(struct rtw_adapter *padapter) pmlmeext->sitesurvey_res.state = SCAN_DISABLE; } - return; } /* collect bss info from Beacon and Probe request/response frames. */ @@ -4760,7 +4756,6 @@ void report_survey_event23a(struct rtw_adapter *padapter, pmlmeext->sitesurvey_res.bss_cnt++; - return; } void report_surveydone_event23a(struct rtw_adapter *padapter) @@ -4803,7 +4798,6 @@ void report_surveydone_event23a(struct rtw_adapter *padapter) rtw_enqueue_cmd23a(pcmdpriv, pcmd_obj); - return; } void report_join_res23a(struct rtw_adapter *padapter, int res) @@ -4851,7 +4845,6 @@ void report_join_res23a(struct rtw_adapter *padapter, int res) rtw_enqueue_cmd23a(pcmdpriv, pcmd_obj); - return; } void report_del_sta_event23a(struct rtw_adapter *padapter, @@ -4907,7 +4900,6 @@ void report_del_sta_event23a(struct rtw_adapter *padapter, rtw_enqueue_cmd23a(pcmdpriv, pcmd_obj); - return; } void report_add_sta_event23a(struct rtw_adapter *padapter, @@ -4952,7 +4944,6 @@ void report_add_sta_event23a(struct rtw_adapter *padapter, rtw_enqueue_cmd23a(pcmdpriv, pcmd_obj); - return; } /**************************************************************************** @@ -5395,7 +5386,6 @@ static void link_timer_hdl(unsigned long data) set_link_timer(pmlmeext, REASSOC_TO); } - return; } static void addba_timer_hdl(unsigned long data)
This patch fixes checkpatch.pl warning in rtw_mlme_ext.c file. WARNING: void function return statements are not generally useful Signed-off-by: Bhaktipriya Shridhar <bhaktipriya96@gmail.com> --- drivers/staging/rtl8723au/core/rtw_mlme_ext.c | 10 ---------- 1 file changed, 10 deletions(-) -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html