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

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

[Rakhi Sharma]

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;
 
-- 
1.9.1

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

[Jes Sorensen]

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

Jes

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

[Joe Perches]

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
	};

	for (i = 0; i < ARRAY_SIZE(dot11_rate_table); i++) {
		if (val == dot11_rate_table[i])
			return BIT(i);
	}

	return 0;
}

Rakhi, do please strive to make the code "better" rather
than merely shut up checkpatch.

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

[Jes Sorensen]

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

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

[Joe Perches]

On Mon, 2016-02-01 at 07:29 -0500, Jes Sorensen wrote:
> Joe Perches <joe@perches.com> writes:
[]
> > 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 };

Either style is OK, but the style I used is slightly
more common in the kernel overall, about 2:1.

The type should ideally be u8 and not char.

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

[Jes Sorensen]

Joe Perches <joe@perches.com> writes:
> On Mon, 2016-02-01 at 07:29 -0500, Jes Sorensen wrote:
>> Joe Perches <joe@perches.com> writes:
> []
>> > 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 };
>
> Either style is OK, but the style I used is slightly
> more common in the kernel overall, about 2:1.

It wastes a line for now reason and having the open bracket on a line by
itself is slightly more obfuscating.

> The type should ideally be u8 and not char.

u8 or char is not a big deal for me here, I'm fine with either.

Jes