[PATCH] staging: r8188eu: refactor rtw_is_cckrates{only}_included()

[PATCH] staging: r8188eu: refactor rtw_is_cckrates{only}_included()

[Michael Straube]

Refactor functions rtw_is_cckrates_included() and
rtw_is_cckratesonly_included(). Add new helper function rtw_is_cckrate()
that allows to make the code more compact. Improves readability and
slightly reduces object file size. Change the return type to bool to
reflect that the functions return boolean values.

Suggested-by: Joe Perches <joe@perches.com>
Signed-off-by: Michael Straube <straube.linux@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_ieee80211.c | 27 +++++++++++---------
 drivers/staging/r8188eu/include/ieee80211.h  |  5 ++--
 2 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_ieee80211.c b/drivers/staging/r8188eu/core/rtw_ieee80211.c
index 0c7231cefdda..892ffcd92cc7 100644
--- a/drivers/staging/r8188eu/core/rtw_ieee80211.c
+++ b/drivers/staging/r8188eu/core/rtw_ieee80211.c
@@ -68,28 +68,31 @@ int rtw_get_bit_value_from_ieee_value(u8 val)
 	return 0;
 }
 
-uint	rtw_is_cckrates_included(u8 *rate)
+static bool rtw_is_cckrate(u8 rate)
 {
-	u32	i = 0;
+	rate &= 0x7f;
+	return rate == 2 || rate == 4 || rate == 11 || rate == 22;
+}
+
+bool rtw_is_cckrates_included(u8 *rate)
+{
+	u8 r;
 
-	while (rate[i] != 0) {
-		if  ((((rate[i]) & 0x7f) == 2) || (((rate[i]) & 0x7f) == 4) ||
-		     (((rate[i]) & 0x7f) == 11)  || (((rate[i]) & 0x7f) == 22))
+	while ((r = *rate++)) {
+		if (rtw_is_cckrate(r))
 			return true;
-		i++;
 	}
+
 	return false;
 }
 
-uint	rtw_is_cckratesonly_included(u8 *rate)
+bool rtw_is_cckratesonly_included(u8 *rate)
 {
-	u32 i = 0;
+	u8 r;
 
-	while (rate[i] != 0) {
-		if  ((((rate[i]) & 0x7f) != 2) && (((rate[i]) & 0x7f) != 4) &&
-		     (((rate[i]) & 0x7f) != 11)  && (((rate[i]) & 0x7f) != 22))
+	while ((r = *rate++)) {
+		if (!rtw_is_cckrate(r))
 			return false;
-		i++;
 	}
 
 	return true;
diff --git a/drivers/staging/r8188eu/include/ieee80211.h b/drivers/staging/r8188eu/include/ieee80211.h
index bc5b030e9c40..1205d13171b2 100644
--- a/drivers/staging/r8188eu/include/ieee80211.h
+++ b/drivers/staging/r8188eu/include/ieee80211.h
@@ -1206,9 +1206,8 @@ int rtw_generate_ie(struct registry_priv *pregistrypriv);
 
 int rtw_get_bit_value_from_ieee_value(u8 val);
 
-uint	rtw_is_cckrates_included(u8 *rate);
-
-uint	rtw_is_cckratesonly_included(u8 *rate);
+bool rtw_is_cckrates_included(u8 *rate);
+bool rtw_is_cckratesonly_included(u8 *rate);
 
 int rtw_check_network_type(unsigned char *rate, int ratelen, int channel);
 
-- 
2.32.0

Re: [PATCH] staging: r8188eu: refactor rtw_is_cckrates{only}_included()

[Fabio M. De Francesco]

On Monday, August 16, 2021 9:31:25 PM CEST Michael Straube wrote:
> Refactor functions rtw_is_cckrates_included() and
> rtw_is_cckratesonly_included(). Add new helper function rtw_is_cckrate()
> that allows to make the code more compact. Improves readability and
> slightly reduces object file size. Change the return type to bool to
> reflect that the functions return boolean values.
> 
> Suggested-by: Joe Perches <joe@perches.com>
> Signed-off-by: Michael Straube <straube.linux@gmail.com>
> ---
>  drivers/staging/r8188eu/core/rtw_ieee80211.c | 27 +++++++++++---------
>  drivers/staging/r8188eu/include/ieee80211.h  |  5 ++--
>  2 files changed, 17 insertions(+), 15 deletions(-)

Now that you took into account also the second of the two suggestions by Joe 
Perches (with a further enhancement he added later), I think that this is 
really a good work. Since the first series is gone, here it is a new...

Acked-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>

Thanks,

Fabio

Re: [PATCH] staging: r8188eu: refactor rtw_is_cckrates{only}_included()

[Greg KH]

On Mon, Aug 16, 2021 at 09:31:25PM +0200, Michael Straube wrote:
> Refactor functions rtw_is_cckrates_included() and
> rtw_is_cckratesonly_included(). Add new helper function rtw_is_cckrate()
> that allows to make the code more compact. Improves readability and
> slightly reduces object file size. Change the return type to bool to
> reflect that the functions return boolean values.
> 
> Suggested-by: Joe Perches <joe@perches.com>
> Signed-off-by: Michael Straube <straube.linux@gmail.com>
> ---
>  drivers/staging/r8188eu/core/rtw_ieee80211.c | 27 +++++++++++---------
>  drivers/staging/r8188eu/include/ieee80211.h  |  5 ++--
>  2 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_ieee80211.c b/drivers/staging/r8188eu/core/rtw_ieee80211.c
> index 0c7231cefdda..892ffcd92cc7 100644
> --- a/drivers/staging/r8188eu/core/rtw_ieee80211.c
> +++ b/drivers/staging/r8188eu/core/rtw_ieee80211.c
> @@ -68,28 +68,31 @@ int rtw_get_bit_value_from_ieee_value(u8 val)
>  	return 0;
>  }
>  
> -uint	rtw_is_cckrates_included(u8 *rate)
> +static bool rtw_is_cckrate(u8 rate)
>  {
> -	u32	i = 0;
> +	rate &= 0x7f;
> +	return rate == 2 || rate == 4 || rate == 11 || rate == 22;
> +}
> +
> +bool rtw_is_cckrates_included(u8 *rate)
> +{
> +	u8 r;
>  
> -	while (rate[i] != 0) {
> -		if  ((((rate[i]) & 0x7f) == 2) || (((rate[i]) & 0x7f) == 4) ||
> -		     (((rate[i]) & 0x7f) == 11)  || (((rate[i]) & 0x7f) == 22))
> +	while ((r = *rate++)) {
> +		if (rtw_is_cckrate(r))
>  			return true;
> -		i++;
>  	}
> +
>  	return false;
>  }
>  
> -uint	rtw_is_cckratesonly_included(u8 *rate)
> +bool rtw_is_cckratesonly_included(u8 *rate)
>  {
> -	u32 i = 0;
> +	u8 r;
>  
> -	while (rate[i] != 0) {
> -		if  ((((rate[i]) & 0x7f) != 2) && (((rate[i]) & 0x7f) != 4) &&
> -		     (((rate[i]) & 0x7f) != 11)  && (((rate[i]) & 0x7f) != 22))
> +	while ((r = *rate++)) {

Ick, no.

While it might be fun to play with pointers like this, trying to
determine the precidence issues involved with reading from, and then
incrementing the pointer like this is crazy.

The original was obvious as to how it was walking through the array.
Keep that here.

Remember, we write code for humans first, compliers second.

thanks,

greg k-h

Re: [PATCH] staging: r8188eu: refactor rtw_is_cckrates{only}_included()

[Joe Perches]

On Tue, 2021-08-17 at 19:57 +0200, Greg KH wrote:
> On Mon, Aug 16, 2021 at 09:31:25PM +0200, Michael Straube wrote:
> > Refactor functions rtw_is_cckrates_included() and
> > rtw_is_cckratesonly_included(). Add new helper function rtw_is_cckrate()
> > that allows to make the code more compact. Improves readability and
> > slightly reduces object file size. Change the return type to bool to
> > reflect that the functions return boolean values.
[]
> > diff --git a/drivers/staging/r8188eu/core/rtw_ieee80211.c b/drivers/staging/r8188eu/core/rtw_ieee80211.c
[]
> > +bool rtw_is_cckratesonly_included(u8 *rate)
> >  {
> > -	u32 i = 0;
> > +	u8 r;
> >  
> > 
> > -	while (rate[i] != 0) {
> > -		if  ((((rate[i]) & 0x7f) != 2) && (((rate[i]) & 0x7f) != 4) &&
> > -		     (((rate[i]) & 0x7f) != 11)  && (((rate[i]) & 0x7f) != 22))
> > +	while ((r = *rate++)) {
> 
> Ick, no.
> 
> While it might be fun to play with pointers like this, trying to
> determine the precidence issues involved with reading from, and then
> incrementing the pointer like this is crazy.
> 
> The original was obvious as to how it was walking through the array.

It's sad to believe *ptr++ is not obvious to you as it's very commonly
used in the kernel sources (over 10,000 instances).

Re: [PATCH] staging: r8188eu: refactor rtw_is_cckrates{only}_included()

[Greg KH]

On Tue, Aug 17, 2021 at 11:36:09AM -0700, Joe Perches wrote:
> On Tue, 2021-08-17 at 19:57 +0200, Greg KH wrote:
> > On Mon, Aug 16, 2021 at 09:31:25PM +0200, Michael Straube wrote:
> > > Refactor functions rtw_is_cckrates_included() and
> > > rtw_is_cckratesonly_included(). Add new helper function rtw_is_cckrate()
> > > that allows to make the code more compact. Improves readability and
> > > slightly reduces object file size. Change the return type to bool to
> > > reflect that the functions return boolean values.
> []
> > > diff --git a/drivers/staging/r8188eu/core/rtw_ieee80211.c b/drivers/staging/r8188eu/core/rtw_ieee80211.c
> []
> > > +bool rtw_is_cckratesonly_included(u8 *rate)
> > >  {
> > > -	u32 i = 0;
> > > +	u8 r;
> > >  
> > > 
> > > -	while (rate[i] != 0) {
> > > -		if  ((((rate[i]) & 0x7f) != 2) && (((rate[i]) & 0x7f) != 4) &&
> > > -		     (((rate[i]) & 0x7f) != 11)  && (((rate[i]) & 0x7f) != 22))
> > > +	while ((r = *rate++)) {
> > 
> > Ick, no.
> > 
> > While it might be fun to play with pointers like this, trying to
> > determine the precidence issues involved with reading from, and then
> > incrementing the pointer like this is crazy.
> > 
> > The original was obvious as to how it was walking through the array.
> 
> It's sad to believe *ptr++ is not obvious to you as it's very commonly
> used in the kernel sources (over 10,000 instances).

There's lots of sad things in life :(

Re: [PATCH] staging: r8188eu: refactor rtw_is_cckrates{only}_included()

[Joe Perches]

On Tue, 2021-08-17 at 20:49 +0200, Greg KH wrote:
> On Tue, Aug 17, 2021 at 11:36:09AM -0700, Joe Perches wrote:
> > On Tue, 2021-08-17 at 19:57 +0200, Greg KH wrote:
> > > On Mon, Aug 16, 2021 at 09:31:25PM +0200, Michael Straube wrote:
> > > > Refactor functions rtw_is_cckrates_included() and
> > > > rtw_is_cckratesonly_included(). Add new helper function rtw_is_cckrate()
> > > > that allows to make the code more compact. Improves readability and
> > > > slightly reduces object file size. Change the return type to bool to
> > > > reflect that the functions return boolean values.
> > []
> > > > diff --git a/drivers/staging/r8188eu/core/rtw_ieee80211.c b/drivers/staging/r8188eu/core/rtw_ieee80211.c
> > []
> > > > +bool rtw_is_cckratesonly_included(u8 *rate)
> > > >  {
> > > > -	u32 i = 0;
> > > > +	u8 r;
> > > >  
> > > > 
> > > > -	while (rate[i] != 0) {
> > > > -		if  ((((rate[i]) & 0x7f) != 2) && (((rate[i]) & 0x7f) != 4) &&
> > > > -		     (((rate[i]) & 0x7f) != 11)  && (((rate[i]) & 0x7f) != 22))
> > > > +	while ((r = *rate++)) {
> > > 
> > > Ick, no.
> > > 
> > > While it might be fun to play with pointers like this, trying to
> > > determine the precidence issues involved with reading from, and then
> > > incrementing the pointer like this is crazy.
> > > 
> > > The original was obvious as to how it was walking through the array.
> > 
> > It's sad to believe *ptr++ is not obvious to you as it's very commonly
> > used in the kernel sources (over 10,000 instances).
> 
> There's lots of sad things in life :(

Your difficulty reading very standard c is relatively low on my list.

Still, it's you not this particular patch to the realtek staging code.

What's really poor about this code is using a 0 terminated list rather
than passing the array size.

But then again, almost all of the realtek code in staging is really poor.

cheers, Joe

Re: [PATCH] staging: r8188eu: refactor rtw_is_cckrates{only}_included()

[Fabio M. De Francesco]

On Tuesday, August 17, 2021 8:49:46 PM CEST Greg KH wrote:
> On Tue, Aug 17, 2021 at 11:36:09AM -0700, Joe Perches wrote:
> > On Tue, 2021-08-17 at 19:57 +0200, Greg KH wrote:
> > > On Mon, Aug 16, 2021 at 09:31:25PM +0200, Michael Straube wrote:
> > > > Refactor functions rtw_is_cckrates_included() and
> > > > rtw_is_cckratesonly_included(). Add new helper function rtw_is_cckrate()
> > > > that allows to make the code more compact. Improves readability and
> > > > slightly reduces object file size. Change the return type to bool to
> > > > reflect that the functions return boolean values.
> > []
> > > > diff --git a/drivers/staging/r8188eu/core/rtw_ieee80211.c b/drivers/staging/r8188eu/core/rtw_ieee80211.c
> > []
> > > > +bool rtw_is_cckratesonly_included(u8 *rate)
> > > >  {
> > > > -	u32 i = 0;
> > > > +	u8 r;
> > > >  
> > > > 
> > > > -	while (rate[i] != 0) {
> > > > -		if  ((((rate[i]) & 0x7f) != 2) && (((rate[i]) & 0x7f) != 4) &&
> > > > -		     (((rate[i]) & 0x7f) != 11)  && (((rate[i]) & 0x7f) != 22))
> > > > +	while ((r = *rate++)) {
> > > 
> > > Ick, no.
> > > 
> > > While it might be fun to play with pointers like this, trying to
> > > determine the precedence issues involved with reading from, and then
> > > incrementing the pointer like this is crazy.
> > > 
> > > The original was obvious as to how it was walking through the array.
> > 
> > It's sad to believe *ptr++ is not obvious to you as it's very commonly
> > used in the kernel sources (over 10,000 instances).
> 
> There's lots of sad things in life :(
> 
Dear Greg,

Please reconsider this issue, I mean it. Let me explain why...

Obviously neither Joe or all the people who knows how much you've given to
Linux during these latest two decades (or is it more?) believes that you have 
any problem with operator precedence :-)

Said that, since operator precedence is one of the first topic that every developer
learn in a course on C and that expressions like *ptr++ are used everywhere in
the kernel you are sending a dangerous message...

It looks like you don't trust people here to be able to do anything more than 
trivial clean-ups. If someone here at linux-staging is not able to understand 
the precedence of operators, please stand up and speak!

We here at linux-staging are not class B developers (compared to A class 
developers of other subsystems). For sure, most of us are newcomers with
less experience than other developers who don't choose to work with
drivers/staging, but you should not prevent us from getting experience and 
using common techniques that are perfectly fine in other areas of Linux.

Thanks for your attention and your precious time,

Fabio
 

Re: [PATCH] staging: r8188eu: refactor rtw_is_cckrates{only}_included()

[Greg KH]

On Wed, Aug 18, 2021 at 08:23:18AM +0200, Fabio M. De Francesco wrote:
> Said that, since operator precedence is one of the first topic that every developer
> learn in a course on C and that expressions like *ptr++ are used everywhere in
> the kernel you are sending a dangerous message...

Operator precedence is something that no one should have to memorize or
remember.  Expressions like *ptr++ on it's own is fine, but combine it
with an assignment and then you need to think about stuff like "did it
increment before or after assigning" and the like.

And really, why?  You are doing nothing to make the code easier to
maintain by doing this.  The compiler isn't going to magically make
better code because of this.  Be explicit and obvious about what you
want the code to do, because in 10+ years when you have to look at it
again to find where to fix a problem, you want it to be really obvious
what is happening.

> It looks like you don't trust people here to be able to do anything more than 
> trivial clean-ups. If someone here at linux-staging is not able to understand 
> the precedence of operators, please stand up and speak!

I want kernel code to be simple and obvious and easy to maintain.

And yes, I do NOT remember the precedence of all C operators, and no one
should be forced to either.  And I am someone who has read or written C
code for almost every day for the past 30 years.

So keep the code simple and obvious for everyone to read and understand.
The original use of the array and then moving to the next one was just
that, simple and obvious.

Do not do unneeded optimizations just because you can, it will always
come back to hurt you, or someone else, in the end.

thanks,

greg k-h