2016-01-29 17:29:03

by Bhaktipriya Shridhar

[permalink] [raw]
Subject: [PATCH] staging: rtl8723au: Fixes unnecessary return warning

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 <[email protected]>
---
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;
}

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)
--
2.1.4



2016-01-29 18:00:42

by Jes Sorensen

[permalink] [raw]
Subject: Re: [PATCH] staging: rtl8723au: Fixes unnecessary return warning

Bhaktipriya Shridhar <[email protected]> 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 <[email protected]>
> ---
> 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

2016-01-30 07:24:57

by Joe Perches

[permalink] [raw]
Subject: Re: [PATCH] staging: rtl8723au: Fixes unnecessary return warning

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.

2016-01-30 03:18:16

by Joe Perches

[permalink] [raw]
Subject: Re: [PATCH] staging: rtl8723au: Fixes unnecessary return warning

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 <[email protected]>
> 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 <[email protected]> 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

2016-01-31 14:32:01

by Jes Sorensen

[permalink] [raw]
Subject: Re: [PATCH] staging: rtl8723au: Fixes unnecessary return warning

Julian Calaby <[email protected]> writes:
> Hi Bhaktipriya,
>
> On Sat, Jan 30, 2016 at 5:00 AM, Jes Sorensen <[email protected]> wrote:
>> Bhaktipriya Shridhar <[email protected]> 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

2016-01-30 03:09:28

by Julian Calaby

[permalink] [raw]
Subject: Re: [PATCH] staging: rtl8723au: Fixes unnecessary return warning

Hi Joe,

On Sat, Jan 30, 2016 at 12:28 PM, Joe Perches <[email protected]> wrote:
> On Sat, 2016-01-30 at 10:17 +1100, Julian Calaby wrote:
>> On Sat, Jan 30, 2016 at 5:00 AM, Jes Sorensen <[email protected]> wrote:
>> > Bhaktipriya Shridhar <[email protected]> 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,

--
Julian Calaby

Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/

2016-01-30 12:09:38

by Joe Perches

[permalink] [raw]
Subject: Re: [PATCH] staging: rtl8723au: Fixes unnecessary return warning

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.


2016-01-30 01:28:29

by Joe Perches

[permalink] [raw]
Subject: Re: [PATCH] staging: rtl8723au: Fixes unnecessary return warning

On Sat, 2016-01-30 at 10:17 +1100, Julian Calaby wrote:
> On Sat, Jan 30, 2016 at 5:00 AM, Jes Sorensen <[email protected]> wrote:
> > Bhaktipriya Shridhar <[email protected]> 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.


2016-01-30 06:53:54

by Bhaktipriya Shridhar

[permalink] [raw]
Subject: Re: [PATCH] staging: rtl8723au: Fixes unnecessary return warning

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 <[email protected]> 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 <[email protected]>
>> 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 <[email protected]> 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

2016-01-29 23:17:20

by Julian Calaby

[permalink] [raw]
Subject: Re: [PATCH] staging: rtl8723au: Fixes unnecessary return warning

Hi Bhaktipriya,

On Sat, Jan 30, 2016 at 5:00 AM, Jes Sorensen <[email protected]> wrote:
> Bhaktipriya Shridhar <[email protected]> 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 <[email protected]>
>> ---
>> 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,

--
Julian Calaby

Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/

2016-01-30 12:02:34

by Julian Calaby

[permalink] [raw]
Subject: Re: [PATCH] staging: rtl8723au: Fixes unnecessary return warning

Hi Bhakti,

On Sat, Jan 30, 2016 at 5:53 PM, Bhakti Priya <[email protected]> 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,

--
Julian Calaby

Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/