Fix line too long warning

PATCH Superseded Kien Ha 3 participants 7 messages Oct 29, 2017

Subsystems (2)

Staging Checkpatch

lore

Share & tools

lore

Kien Ha

author

2017-10-29 02:46 UTC #0 lore raw

PATCH 1/1 Fix line too long warning

Quoted 1 line

From fc52a98aca0c033f2c03fdc7e8f83ae49625675a Mon Sep 17 00:00:00 2001

From: Kien Ha <[email protected]>
Date: Fri, 27 Oct 2017 14:07:55 -0400
Subject: [PATCH] Fix line too long warning

Signed-off-by Kien Ha <[email protected]>

---
 drivers/staging/rtlwifi/base.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/staging/rtlwifi/base.c +2 -1

index b88b0e8edd3d..bbc80f976e12 100644
@@ -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];

Signature · 1 line

2.14.3

Yury Norov

2017-10-29 15:54 UTC #2 lore raw

In reply to Kien Ha

Hi Kien,

Quoted 15 lines · Kien Ha

Quoted 1 line
From fc52a98aca0c033f2c03fdc7e8f83ae49625675a Mon Sep 17 00:00:00 2001
From: Kien Ha <[email protected]>
Date: Fri, 27 Oct 2017 14:07:55 -0400
Subject: [PATCH] Fix line too long warning
Signed-off-by Kien Ha <[email protected]>
---
 drivers/staging/rtlwifi/base.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/staging/rtlwifi/base.c +0 -0
index b88b0e8edd3d..bbc80f976e12 100644

drivers/staging/rtlwifi/base.c void rtl_get_tcb_desc(struct ieee80211_hw *hw, · line 1287

Quoted 6 lines

drivers/staging/rtlwifi/base.c +2 -1

@@ -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?

drivers/staging/rtlwifi/base.c void rtl_get_tcb_desc(struct ieee80211_hw *hw, · line 1290

Quoted 3 lines

drivers/staging/rtlwifi/base.c +0 -0

 					} 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

Yury Norov

2017-10-29 16:29 UTC #3 lore raw

Quoted 39 lines · Yury Norov

Hi Kien,
Quoted 15 lines · Kien Ha
Quoted 1 line
From fc52a98aca0c033f2c03fdc7e8f83ae49625675a Mon Sep 17 00:00:00 2001
From: Kien Ha <[email protected]>
Date: Fri, 27 Oct 2017 14:07:55 -0400
Subject: [PATCH] Fix line too long warning
Signed-off-by Kien Ha <[email protected]>
---
 drivers/staging/rtlwifi/base.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/staging/rtlwifi/base.c +0 -0
index b88b0e8edd3d..bbc80f976e12 100644
drivers/staging/rtlwifi/base.c void rtl_get_tcb_desc(struct ieee80211_hw *hw, · line 1287
Quoted 6 lines
drivers/staging/rtlwifi/base.c +2 -1
@@ -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?
drivers/staging/rtlwifi/base.c void rtl_get_tcb_desc(struct ieee80211_hw *hw, · line 1290
Quoted 3 lines
drivers/staging/rtlwifi/base.c +0 -0
 					} 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.

Quoted 2 lines

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

Kien Ha

author

2017-10-29 17:22 UTC #4 lore raw

In reply to Yury Norov

Quoted 55 lines · Yury Norov

Hi Kien,
Quoted 29 lines · Kien Ha
Quoted 2 lines
From fc52a98aca0c033f2c03fdc7e8f83ae49625675a Mon Sep 17 00:00:00 2001
From: Kien Ha <[email protected]>
Date: Fri, 27 Oct 2017 14:07:55 -0400
Subject: [PATCH] Fix line too long warning
Signed-off-by Kien Ha <[email protected]>
---
 drivers/staging/rtlwifi/base.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/staging/rtlwifi/base.c +0 -0
index b88b0e8edd3d..bbc80f976e12 100644
@@ -1283,7 +1283,8 @@ void rtl_get_tcb_desc(struct ieee80211_hw
 *hw,
 				} else {
 					if (rtlmac->mode ==
 WIRELESS_MODE_B) {
 						tcb_desc->hw_rate
=
drivers/staging/rtlwifi/base.c +0 -1
-						    rtlpriv->cfg-
Quoted 1 line
maps[RTL_RC_CCK_RATE11M];
drivers/staging/rtlwifi/base.c +1 -0
+						    rtlpriv->cfg-
Quoted 1 line
maps[
drivers/staging/rtlwifi/base.c +1 -0
+						    RTL_RC_CCK_RAT
Quoted 1 line
E11M];
At first, if you fix this, you should also fix similar problem 3 lines
below, right?
Quoted 5 lines
drivers/staging/rtlwifi/base.c +0 -0
 					} else {
 						tcb_desc->hw_rate
=
rtlpriv->cfg-
Quoted 1 line
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.

Quoted 5 lines

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.

Joe Perches

maintainer

2017-10-29 17:28 UTC #5 lore raw

In reply to Yury Norov

Quoted 9 lines · Yury Norov

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(-)

scripts/checkpatch.pl +1 -1

index 6bdd43d5dec5..923e4ff09d24 100755
@@ -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);
 			}

Yury Norov

Acked-by

2017-10-29 18:35 UTC #6 lore raw

Quoted 59 lines · Joe Perches

Quoted 9 lines · Yury Norov
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(-)
scripts/checkpatch.pl +0 -0
index 6bdd43d5dec5..923e4ff09d24 100755

scripts/checkpatch.pl sub process { · line 3359

Quoted 8 lines

scripts/checkpatch.pl +1 -1

@@ -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 <[email protected]>