staging: r8188eu: refactor rtw_is_cckrates{only}_included()
PATCH 1/1 staging: r8188eu: refactor rtw_is_cckrates{only}_included()
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.
--- drivers/staging/r8188eu/core/rtw_ieee80211.c | 27 +++++++++++--------- drivers/staging/r8188eu/include/ieee80211.h | 5 ++-- 2 files changed, 17 insertions(+), 15 deletions(-)
drivers/staging/r8188eu/core/rtw_ieee80211.c +15 -12
@@ -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;
drivers/staging/r8188eu/include/ieee80211.h +2 -3
@@ -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);
Signature · 1 line
2.32.0
Quoted 6 lines · CEST Michael Straube
Refactor functions
rtw_is_cckrates_included()andrtw_is_cckratesonly_included(). Add new helper functionrtw_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.
Quoted 6 lines
Suggested-by Joe Perches <[email protected]>Signed-off-by Michael Straube <[email protected]>--- 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...
Thanks,
Fabio
Greg KH
maintainerQuoted 45 lines · Michael Straube
Refactor functions
rtw_is_cckrates_included()andrtw_is_cckratesonly_included(). Add new helper functionrtw_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 <[email protected]>Signed-off-by Michael Straube <[email protected]>--- drivers/staging/r8188eu/core/rtw_ieee80211.c | 27 +++++++++++--------- drivers/staging/r8188eu/include/ieee80211.h | 5 ++-- 2 files changed, 17 insertions(+), 15 deletions(-)drivers/staging/r8188eu/core/rtw_ieee80211.c +11
-6@@ -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; }
drivers/staging/r8188eu/core/rtw_ieee80211.c int rtw_get_bit_value_from_ieee_value(u8 val) · line 93
Quoted 10 lines
drivers/staging/r8188eu/core/rtw_ieee80211.c +3
-5-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
Quoted 6 lines · Greg KH
Quoted 5 lines · Michael Straube
Refactor functions
rtw_is_cckrates_included()andrtw_is_cckratesonly_included(). Add new helper functionrtw_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.
[]
Quoted 1 line
drivers/staging/r8188eu/core/rtw_ieee80211.c
Quoted 1 line
drivers/staging/r8188eu/core/rtw_ieee80211.c +0
-0
[]
Quoted 13 lines
drivers/staging/r8188eu/core/rtw_ieee80211.c int rtw_get_bit_value_from_ieee_value(u8 val) · line 93
Quoted 10 lines
+bool
rtw_is_cckratesonly_included(u8 *rate)
{drivers/staging/r8188eu/core/rtw_ieee80211.c +1
-1-u32 i =0 ; +u8 r;drivers/staging/r8188eu/core/rtw_ieee80211.c +1
-3-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.
Quoted 5 lines
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).
Greg KH
maintainerQuoted 23 lines · Joe Perches
Quoted 6 lines · Greg KH
Quoted 5 lines · Michael Straube
Refactor functions
rtw_is_cckrates_included()andrtw_is_cckratesonly_included(). Add new helper functionrtw_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.[]
Quoted 1 line
drivers/staging/r8188eu/core/rtw_ieee80211.c
Quoted 1 line
drivers/staging/r8188eu/core/rtw_ieee80211.c +0
-0[]
Quoted 13 lines
drivers/staging/r8188eu/core/rtw_ieee80211.c int rtw_get_bit_value_from_ieee_value(u8 val) · line 93
Quoted 10 lines
+bool
rtw_is_cckratesonly_included(u8 *rate)
{drivers/staging/r8188eu/core/rtw_ieee80211.c +1
-1-u32 i =0 ; +u8 r;drivers/staging/r8188eu/core/rtw_ieee80211.c +1
-3-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.
Quoted 8 lines
Quoted 5 lines
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 :(
Quoted 28 lines · Greg KH
Quoted 27 lines · Joe Perches
Quoted 6 lines · Greg KH
Quoted 5 lines · Michael Straube
Refactor functions
rtw_is_cckrates_included()andrtw_is_cckratesonly_included(). Add new helper functionrtw_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.[]
Quoted 1 line
drivers/staging/r8188eu/core/rtw_ieee80211.c
Quoted 1 line
drivers/staging/r8188eu/core/rtw_ieee80211.c +0
-0[]
Quoted 17 lines
drivers/staging/r8188eu/core/rtw_ieee80211.c int rtw_get_bit_value_from_ieee_value(u8 val) · line 93
Quoted 10 lines
+bool
rtw_is_cckratesonly_included(u8 *rate)
{drivers/staging/r8188eu/core/rtw_ieee80211.c +1
-1-u32 i =0 ; +u8 r;drivers/staging/r8188eu/core/rtw_ieee80211.c +1
-3-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.
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
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
Quoted 28 lines · CEST Greg KH
Quoted 27 lines · Joe Perches
Quoted 6 lines · Greg KH
Quoted 5 lines · Michael Straube
Refactor functions
rtw_is_cckrates_included()andrtw_is_cckratesonly_included(). Add new helper functionrtw_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.[]
Quoted 1 line
drivers/staging/r8188eu/core/rtw_ieee80211.c
Quoted 1 line
drivers/staging/r8188eu/core/rtw_ieee80211.c +0
-0[]
Quoted 17 lines
drivers/staging/r8188eu/core/rtw_ieee80211.c int rtw_get_bit_value_from_ieee_value(u8 val) · line 93
Quoted 10 lines
drivers/staging/r8188eu/core/rtw_ieee80211.c +3
-4+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.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
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
Greg KH
maintainerQuoted 3 lines · Fabio M. De Francesco
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.
Quoted 3 lines
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