Re: [PATCH 3/3] Staging: rtl8723au: core: rtw_ieee80211: Fixed space and brace coding style issue.

[Jes Sorensen <Jes.Sorensen@redhat.com>]

Joe Perches <joe@perches.com> writes:
> On Sun, 2016-01-31 at 09:36 -0500, Jes Sorensen wrote:
>> Rakhi Sharma <rakhish1994@gmail.com> writes:
>> > Fixed the space and brace coding style error.
>> > ERROR: space required before that '='
>> > ERROR: that open brace { should be on the previous line.
>> > 
>> > Signed-off-by: Rakhi Sharma <rakhish1994@gmail.com>
>> > ---
>> >  drivers/staging/rtl8723au/core/rtw_ieee80211.c | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> > 
>> > diff --git a/drivers/staging/rtl8723au/core/rtw_ieee80211.c
>> > b/drivers/staging/rtl8723au/core/rtw_ieee80211.c
>> > index 3b0d782..0c933e4 100644
>> > --- a/drivers/staging/rtl8723au/core/rtw_ieee80211.c
>> > +++ b/drivers/staging/rtl8723au/core/rtw_ieee80211.c
>> > @@ -65,8 +65,8 @@ static u8 WIFI_OFDMRATES[] = {
>> >  
>> >  int rtw_get_bit_value_from_ieee_value23a(u8 val)
>> >  {
>> > -	unsigned char dot11_rate_table[] =
>> > -			{2, 4, 11, 22, 12, 18, 24, 36, 48, 72, 96, 108, 0};
>> > +	unsigned char dot11_rate_table[] = {
>> > +		2, 4, 11, 22, 12, 18, 24, 36, 48, 72, 96, 108, 0};
>> >  
>> >  	int i = 0;
>> 
>> This is a great example of checkpatch doing the wrong thing. What you
>> did here was to make the code uglier rather than better.
>> 
>> NACK
>
> Here's the original code:
>
> int rtw_get_bit_value_from_ieee_value23a(u8 val)
> {
> 	unsigned char dot11_rate_table[]=
> 		{2, 4, 11, 22, 12, 18, 24, 36, 48, 72, 96, 108, 0};
>
> 	int i = 0;
>
> 	while (dot11_rate_table[i] != 0) {
> 		if (dot11_rate_table[i] == val)
> 			return BIT(i);
> 		i++;
> 	}
> 	return 0;
> }
>
> It has several nominal style defects:
>
> o compares different types (unsigned char to u8)
> o uses a while loop and test of a known sized array
> o array isn't declared static const
> o array initialization is different style from others
>   in the same file. "type foo[] = { bar, baz };"
> o the function argument is tested on the right side
>   tested on left side is more common.
>
> So this could be transformed into something like:
>
> int rtw_get_bit_value_from_ieee_value23a(u8 val)
> {
> 	int i;
> 	static const u8 dot11_rate_table[] = {
> 		2, 4, 11, 22, 12, 18, 24, 36, 48, 72, 96, 108
> 	};

and per my previous email, the above is worse than than the original.
The cleaner way to list it would be:

	static const char dot11_rate_table[] =
		{ 2, 4, 11, 22, 12, 18, 24, 36, 48, 72, 96, 108 };

Jes