[PATCH] Fix line too long warning

[PATCH] Fix line too long warning

[Kien Ha]

>From fc52a98aca0c033f2c03fdc7e8f83ae49625675a Mon Sep 17 00:00:00 2001
From: Kien Ha <kienha9922@gmail.com>
Date: Fri, 27 Oct 2017 14:07:55 -0400
Subject: [PATCH] Fix line too long warning

Signed-off-by: Kien Ha <kienha9922@gmail.com>
---
 drivers/staging/rtlwifi/base.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/rtlwifi/base.c b/drivers/staging/rtlwifi/base.c
index b88b0e8edd3d..bbc80f976e12 100644
--- a/drivers/staging/rtlwifi/base.c
+++ b/drivers/staging/rtlwifi/base.c
@@ -1283,7 +1283,8 @@ void rtl_get_tcb_desc(struct ieee80211_hw *hw,
 				} else {
 					if (rtlmac->mode == WIRELESS_MODE_B) {
 						tcb_desc->hw_rate =
-						    rtlpriv->cfg->maps[RTL_RC_CCK_RATE11M];
+						    rtlpriv->cfg->maps[
+						    RTL_RC_CCK_RATE11M];
 					} else {
 						tcb_desc->hw_rate =
 						    rtlpriv->cfg->maps[RTL_RC_OFDM_RATE54M];
-- 
2.14.3

Re: [PATCH] Fix line too long warning

[Joe Perches]

On Sat, 2017-10-28 at 22:46 -0400, Kien Ha wrote:
> From fc52a98aca0c033f2c03fdc7e8f83ae49625675a Mon Sep 17 00:00:00 2001
> From: Kien Ha <kienha9922@gmail.com>
> Date: Fri, 27 Oct 2017 14:07:55 -0400
> Subject: [PATCH] Fix line too long warning
> 
> Signed-off-by: Kien Ha <kienha9922@gmail.com>

Hi.

Instead of merely shutting up checkpatch warnings,
always try to improve the code instead.

Look at the flow of this cascaded if / else { if
and transform it to a simpler if ... else if ... else if ...

                       if (sta && sta->vht_cap.vht_supported) {
                               tcb_desc->hw_rate =
                               _rtl_get_vht_highest_n_rate(hw, sta);
                       } else {
                               if (sta && (sta->ht_cap.ht_supported)) {
                                       tcb_desc->hw_rate =
                                           _rtl_get_highest_n_rate(hw, sta);
                               } else {
                                       if (rtlmac->mode == WIRELESS_MODE_B) {
                                               tcb_desc->hw_rate =
                                                   rtlpriv->cfg->maps[RTL_RC_CCK_RATE11M];
                                       } else {
                                               tcb_desc->hw_rate =
                                                   rtlpriv->cfg->maps[RTL_RC_OFDM_RATE54M];
                                       }
                               }
                       }


This block could be

		tcb_desc->hw_rate =
			sta && sta->vht_cap.vht_supported ?
				_rtl_get_vht_highest_n_rate(hw, sta) :
			sta && sta->ht_cap.ht_supported ?
				_rtl_get_highest_n_rate(hw, sta) :
			rtlmac->mode == WIRELESS_MODE_B ?
				rtlpriv->cfg->maps[RTL_RC_CCK_RATE11M] :
				rtlpriv->cfg->maps[RTL_RC_OFDM_RATE54M];

which seems easier to read.

Re: [PATCH] Fix line too long warning

[Yury Norov]

Hi Kien,

On Sat, Oct 28, 2017 at 10:46:13PM -0400, Kien Ha wrote:
> >From fc52a98aca0c033f2c03fdc7e8f83ae49625675a Mon Sep 17 00:00:00 2001
> From: Kien Ha <kienha9922@gmail.com>
> Date: Fri, 27 Oct 2017 14:07:55 -0400
> Subject: [PATCH] Fix line too long warning
> 
> Signed-off-by: Kien Ha <kienha9922@gmail.com>
> ---
>  drivers/staging/rtlwifi/base.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/rtlwifi/base.c b/drivers/staging/rtlwifi/base.c
> index b88b0e8edd3d..bbc80f976e12 100644
> --- a/drivers/staging/rtlwifi/base.c
> +++ b/drivers/staging/rtlwifi/base.c
> @@ -1283,7 +1283,8 @@ void rtl_get_tcb_desc(struct ieee80211_hw *hw,
>  				} else {
>  					if (rtlmac->mode == WIRELESS_MODE_B) {
>  						tcb_desc->hw_rate =
> -						    rtlpriv->cfg->maps[RTL_RC_CCK_RATE11M];
> +						    rtlpriv->cfg->maps[
> +						    RTL_RC_CCK_RATE11M];

At first, if you fix this, you should also fix similar problem 3 lines
below, right?

>  					} else {
>  						tcb_desc->hw_rate =
>  						    rtlpriv->cfg->maps[RTL_RC_OFDM_RATE54M];

At second, and most important, refer Documentation/process/coding-style.rst:
Now, some people will claim that having 8-character indentations makes
the code move too far to the right, and makes it hard to read on a 
80-character terminal screen.  The answer to that is that if you need 
more than 3 levels of indentation, you're screwed anyway, and should fix
your program.

The real problem here is not "line too long", but "indentation level too
big" - 5. And it worth to address real problem.

Yury

Re: [PATCH] Fix line too long warning

[Yury Norov]

On Sun, Oct 29, 2017 at 06:54:09PM +0300, Yury Norov wrote:
> Hi Kien,
> 
> On Sat, Oct 28, 2017 at 10:46:13PM -0400, Kien Ha wrote:
> > >From fc52a98aca0c033f2c03fdc7e8f83ae49625675a Mon Sep 17 00:00:00 2001
> > From: Kien Ha <kienha9922@gmail.com>
> > Date: Fri, 27 Oct 2017 14:07:55 -0400
> > Subject: [PATCH] Fix line too long warning
> > 
> > Signed-off-by: Kien Ha <kienha9922@gmail.com>
> > ---
> >  drivers/staging/rtlwifi/base.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/rtlwifi/base.c b/drivers/staging/rtlwifi/base.c
> > index b88b0e8edd3d..bbc80f976e12 100644
> > --- a/drivers/staging/rtlwifi/base.c
> > +++ b/drivers/staging/rtlwifi/base.c
> > @@ -1283,7 +1283,8 @@ void rtl_get_tcb_desc(struct ieee80211_hw *hw,
> >  				} else {
> >  					if (rtlmac->mode == WIRELESS_MODE_B) {
> >  						tcb_desc->hw_rate =
> > -						    rtlpriv->cfg->maps[RTL_RC_CCK_RATE11M];
> > +						    rtlpriv->cfg->maps[
> > +						    RTL_RC_CCK_RATE11M];
> 
> At first, if you fix this, you should also fix similar problem 3 lines
> below, right?
> 
> >  					} else {
> >  						tcb_desc->hw_rate =
> >  						    rtlpriv->cfg->maps[RTL_RC_OFDM_RATE54M];
> 
> At second, and most important, refer Documentation/process/coding-style.rst:
> Now, some people will claim that having 8-character indentations makes
> the code move too far to the right, and makes it hard to read on a 
> 80-character terminal screen.  The answer to that is that if you need 
> more than 3 levels of indentation, you're screwed anyway, and should fix
> your program.
> 
> The real problem here is not "line too long", but "indentation level too
> big" - 5. And it worth to address real problem.

It's not so hard though, something like this:

void rtl_get_tcb_desc(struct ieee80211_hw *hw,
		      struct ieee80211_tx_info *info,
		      struct ieee80211_sta *sta,
		      struct sk_buff *skb, struct rtl_tcb_desc *tcb_desc)
{
#define SET_RATE_ID(rate_id)					\
	((rtlpriv->cfg->spec_ver & RTL_SPEC_NEW_RATEID) ?	\
		rtl_mrate_idx_to_arfr_id(hw, rate_id,		\
			(sta_entry ? sta_entry->wireless_mode :	\
			 WIRELESS_MODE_G)) :			\
		rate_id)

	struct rtl_priv *rtlpriv = rtl_priv(hw);
	struct rtl_mac *rtlmac = rtl_mac(rtl_priv(hw));
	struct ieee80211_hdr *hdr = rtl_get_hdr(skb);
	struct rtl_sta_info *sta_entry =
		(sta ? (struct rtl_sta_info *)sta->drv_priv : NULL);

	__le16 fc = rtl_get_fc(skb);

	tcb_desc->hw_rate = _rtl_get_tx_hw_rate(hw, info);

	if (rtl_is_tx_report_skb(hw, skb))
		tcb_desc->use_spe_rpt = 1;

	if (!ieee80211_is_data(fc)) {
		tcb_desc->use_driver_rate = true;
		tcb_desc->ratr_index = SET_RATE_ID(RATR_INX_WIRELESS_MC);
		tcb_desc->disable_ratefallback = 1;
		tcb_desc->mac_id = 0;
		tcb_desc->packet_bw = false;
		return;
	}

	if (is_multicast_ether_addr(hdr->addr1))
		tcb_desc->multicast = 1;
	else if (is_broadcast_ether_addr(hdr->addr1))
		tcb_desc->broadcast = 1;

	/*
	 * we set data rate INX 0 in rtl_rc.c if skb is special data or
	 * mgt which need low data rate. So tcb_desc->hw_rate is just used
	 * for special data and mgt frames
	 */
	if (info->control.rates[0].idx == 0 || ieee80211_is_nullfunc(fc)) {
		tcb_desc->use_driver_rate = true;
		tcb_desc->ratr_index = SET_RATE_ID(RATR_INX_WIRELESS_MC);
		tcb_desc->disable_ratefallback = 1;
		goto rtl_query;
	}

	/* because hw will never use hw_rate
	 * when tcb_desc->use_driver_rate = false
	 * so we never set highest N rate here,
	 * and N rate will all be controlled by FW
	 * when tcb_desc->use_driver_rate = false
	 */
	if (sta && sta->vht_cap.vht_supported) {
		tcb_desc->hw_rate = _rtl_get_vht_highest_n_rate(hw, sta);
		goto rtl_query;
	}

	if (sta && (sta->ht_cap.ht_supported)) {
		tcb_desc->hw_rate = _rtl_get_highest_n_rate(hw, sta);
		goto rtl_query;
	}

	if (rtlmac->mode == WIRELESS_MODE_B)
		tcb_desc->hw_rate = rtlpriv->cfg->maps[RTL_RC_CCK_RATE11M];
	else
		tcb_desc->hw_rate = rtlpriv->cfg->maps[RTL_RC_OFDM_RATE54M];
rtl_query:
	_rtl_txrate_selectmode(hw, sta, tcb_desc);
	_rtl_query_bandwidth_mode(hw, sta, tcb_desc);
	_rtl_qurey_shortpreamble_mode(hw, tcb_desc, info);
	_rtl_query_shortgi(hw, sta, tcb_desc, info);
	_rtl_query_protection_mode(hw, tcb_desc, info);
#undef SET_RATE_ID
}

(Compeletely untested and most probably wrong, just illustration.)

Yury

Re: [PATCH] Fix line too long warning

[Kien Ha]

On Sun, 2017-10-29 at 18:54 +0300, Yury Norov wrote:
> Hi Kien,
> 
> On Sat, Oct 28, 2017 at 10:46:13PM -0400, Kien Ha wrote:
> > > From fc52a98aca0c033f2c03fdc7e8f83ae49625675a Mon Sep 17 00:00:00
> > > 2001
> > 
> > From: Kien Ha <kienha9922@gmail.com>
> > Date: Fri, 27 Oct 2017 14:07:55 -0400
> > Subject: [PATCH] Fix line too long warning
> > 
> > Signed-off-by: Kien Ha <kienha9922@gmail.com>
> > ---
> >  drivers/staging/rtlwifi/base.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/rtlwifi/base.c
> > b/drivers/staging/rtlwifi/base.c
> > index b88b0e8edd3d..bbc80f976e12 100644
> > --- a/drivers/staging/rtlwifi/base.c
> > +++ b/drivers/staging/rtlwifi/base.c
> > @@ -1283,7 +1283,8 @@ void rtl_get_tcb_desc(struct ieee80211_hw
> > *hw,
> >  				} else {
> >  					if (rtlmac->mode ==
> > WIRELESS_MODE_B) {
> >  						tcb_desc->hw_rate
> > =
> > -						    rtlpriv->cfg-
> > >maps[RTL_RC_CCK_RATE11M];
> > +						    rtlpriv->cfg-
> > >maps[
> > +						    RTL_RC_CCK_RAT
> > E11M];
> 
> At first, if you fix this, you should also fix similar problem 3
> lines
> below, right?
> 
> >  					} else {
> >  						tcb_desc->hw_rate
> > =
> >  						    rtlpriv->cfg-
> > >maps[RTL_RC_OFDM_RATE54M];
> 
> At second, and most important, refer Documentation/process/coding-
> style.rst:
> Now, some people will claim that having 8-character indentations
> makes
> the code move too far to the right, and makes it hard to read on a 
> 80-character terminal screen.  The answer to that is that if you
> need 
> more than 3 levels of indentation, you're screwed anyway, and should
> fix
> your program.
> 
> The real problem here is not "line too long", but "indentation level
> too
> big" - 5. And it worth to address real problem.
> 
> Yury

Thank you for taking some of your time to review my patch, providing
valuable feedback and pointing me in the right direction Joe and Yury.
I submitted another patch with improved code based on your
suggestions.

Re: [PATCH] Fix line too long warning

[Joe Perches]

On Sun, 2017-10-29 at 18:54 +0300, Yury Norov wrote:
> At second, and most important, refer Documentation/process/coding-style.rst:
> Now, some people will claim that having 8-character indentations makes
> the code move too far to the right, and makes it hard to read on a 
> 80-character terminal screen.  The answer to that is that if you need 
> more than 3 levels of indentation, you're screwed anyway, and should fix
> your program.
> 
> The real problem here is not "line too long", but "indentation level too
> big" - 5. And it worth to address real problem.

Line length issues can be a combination of several factors:

o identifier length
o quantity of dereferences
o indentation depth
o code complexity

4 indentation depth levels are not a real issue.
A significant percentage of lines in the kernel
are 4 or more tab indent levels deep.

checkpatch suggests that 6 or more is the depth level
that should cause real concern.

Here's a little breakdown of lines that start with
a tab followed by a c90 keyword in the kernel

$ git grep -P "^\t+(if|for|do|while|\}|else|switch|return|case|break|continue|goto)\b" -- "*.[ch]" | \
  cut -f2- -d":" | perl -p -e 's/(^\t+).*/\1/' | \
  sort | uniq -c | sort -rn | \
  awk '{total += $1; count[i++] = $1} END { for (j = 0; j < i; j++) { printf "%d\t%d\t%.2f%%\n", j + 1, count[j], count[j] / total * 100 }}'
1	1325462	52.19%
2	863007	33.98%
3	271844	10.70%
4	64009	2.52%
5	12502	0.49%
6	2199	0.09%
7	501	0.02%
8	166	0.01%
9	51	0.00%
10	20	0.00%
11	10	0.00%
12	4	0.00%
13	1	0.00%

I think it could reasonably be argued that the
indentation depth warning (DEEP_INDENTATION)
should start at 5 and not at 6.

---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 6bdd43d5dec5..923e4ff09d24 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3353,7 +3353,7 @@ sub process {
 
 			my ($level, @ctx) = ctx_statement_level($linenr, $realcnt, 0);
 
-			if ($line =~ /^\+\t{6,}/) {
+			if ($line =~ /^\+\t{5,}/) {
 				WARN("DEEP_INDENTATION",
 				     "Too many leading tabs - consider code refactoring\n" . $herecurr);
 			}

Re: [PATCH] Fix line too long warning

[Yury Norov]

On Sun, Oct 29, 2017 at 10:28:27AM -0700, Joe Perches wrote:
> On Sun, 2017-10-29 at 18:54 +0300, Yury Norov wrote:
> > At second, and most important, refer Documentation/process/coding-style.rst:
> > Now, some people will claim that having 8-character indentations makes
> > the code move too far to the right, and makes it hard to read on a 
> > 80-character terminal screen.  The answer to that is that if you need 
> > more than 3 levels of indentation, you're screwed anyway, and should fix
> > your program.
> > 
> > The real problem here is not "line too long", but "indentation level too
> > big" - 5. And it worth to address real problem.
> 
> Line length issues can be a combination of several factors:
> 
> o identifier length
> o quantity of dereferences
> o indentation depth
> o code complexity
> 
> 4 indentation depth levels are not a real issue.
> A significant percentage of lines in the kernel
> are 4 or more tab indent levels deep.
> 
> checkpatch suggests that 6 or more is the depth level
> that should cause real concern.
> 
> Here's a little breakdown of lines that start with
> a tab followed by a c90 keyword in the kernel
> 
> $ git grep -P "^\t+(if|for|do|while|\}|else|switch|return|case|break|continue|goto)\b" -- "*.[ch]" | \
>   cut -f2- -d":" | perl -p -e 's/(^\t+).*/\1/' | \
>   sort | uniq -c | sort -rn | \
>   awk '{total += $1; count[i++] = $1} END { for (j = 0; j < i; j++) { printf "%d\t%d\t%.2f%%\n", j + 1, count[j], count[j] / total * 100 }}'
> 1	1325462	52.19%
> 2	863007	33.98%
> 3	271844	10.70%
> 4	64009	2.52%
> 5	12502	0.49%
> 6	2199	0.09%
> 7	501	0.02%
> 8	166	0.01%
> 9	51	0.00%
> 10	20	0.00%
> 11	10	0.00%
> 12	4	0.00%
> 13	1	0.00%
> 
> I think it could reasonably be argued that the
> indentation depth warning (DEEP_INDENTATION)
> should start at 5 and not at 6.
> 
> ---
>  scripts/checkpatch.pl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 6bdd43d5dec5..923e4ff09d24 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3353,7 +3353,7 @@ sub process {
>  
>  			my ($level, @ctx) = ctx_statement_level($linenr, $realcnt, 0);
>  
> -			if ($line =~ /^\+\t{6,}/) {
> +			if ($line =~ /^\+\t{5,}/) {
>  				WARN("DEEP_INDENTATION",
>  				     "Too many leading tabs - consider code refactoring\n" . $herecurr);
>  			}

There are 2 different subjects here - this specific function in staging
driver and general considerations.

Regarding the function, it has very simple structure, and so deep indentation
level is definitely a symptom of bad design.

Regarding general considerations, the kernel is very complex, and I admit
that indentations deeper than 3 are unavoidable in some cases.

But we have coding style that insists on 3, and this rule is (probably ?)
written by Linus, so... :-)

Nevertheless, your patch is the step toward right direction, so if you
need my ack,
Acked-by: Yury Norov <ynorov@caviumnetworks.com>