here are 3 minor tweaks to checkpatch
- skip reporting of a SPACING test, based on filename exclusion.
*.lds.h are linker scripts, some rules don't apply
- exclude extern-C warning for symbol patterns common to vmlinux.lds.h
(_start|_stop|_end) currently, pls tweak as fitting.
- subdue warning on BUG_ON if BUG_ON is mentioned in commit log
Jim Cromie (3):
checkpatch: skip spacing tests on linker scripts
checkpatch: tweak extern in C warning
checkpatch: suppress BUG_ON warn when it is named in commitmsg
scripts/checkpatch.pl | 27 ++++++++++++++++++++++++---
1 file changed, 24 insertions(+), 3 deletions(-)
--
2.31.1
Before issuing a WARNING on spacing, exclude reports on linker
scripts, which don't comport to C style rules. skip_on_filename()
skips on *.lds.h files, I think covering all cases.
Signed-off-by: Jim Cromie <[email protected]>
---
scripts/checkpatch.pl | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 23697a6b1eaa..4fe9fab20009 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2546,6 +2546,11 @@ sub get_raw_comment {
return $comment;
}
+sub skip_on_filename {
+ my $fname = shift;
+ return $fname =~ m@\.lds\.h$@;
+}
+
sub exclude_global_initialisers {
my ($realfile) = @_;
@@ -5134,7 +5139,8 @@ sub process {
}
}
} elsif ($ctx =~ /Wx[^WCE]|[^WCE]xW/) {
- if (ERROR("SPACING",
+ if (!skip_on_filename($realfile) &&
+ ERROR("SPACING",
"need consistent spacing around '$op' $at\n" . $hereptr)) {
$good = rtrim($fix_elements[$n]) . " " . trim($fix_elements[$n + 1]) . " ";
if (defined $fix_elements[$n + 2]) {
--
2.31.1
The extern-in-C rule has one important exception: the symbol is
defined in/by the linker script. By convention, these almost always
contain: _start, _stop, _end. Suppress the warning on such symbols.
Signed-off-by: Jim Cromie <[email protected]>
---
scripts/checkpatch.pl | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 4fe9fab20009..a8dfba53b593 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6910,7 +6910,8 @@ sub process {
$stat =~ /^.\s*extern\s+/)
{
WARN("AVOID_EXTERNS",
- "externs should be avoided in .c files\n" . $herecurr);
+ "externs should be avoided in .c files\n($stat)\n" . $herecurr)
+ unless $stat =~ /_start|_stop|_end/;
}
# check for function declarations that have arguments without identifier names
--
2.31.1
allow mention of BUG_ON in the preamble/commitmsg/intro to silence the
warning normally issued when one is added. This presumes the commit
message will adequately explain the reason "BUG_ON" is appropriate.
Signed-off-by: Jim Cromie <[email protected]>
---
scripts/checkpatch.pl | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a8dfba53b593..32612f39d742 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2560,6 +2560,18 @@ sub exclude_global_initialisers {
$realfile =~ m@/bpf/.*\.bpf\.c$@;
}
+sub commitmsg_refers_to { # see if $srch is in commit message
+ my ($srch, $lines) = @_;
+ #print "ok checking for $srch in $lines lines\n";
+ for my $i (0..$lines) {
+ if ($rawlines[$i] =~ /$srch/) {
+ print "\thmm: $srch mentioned in preamble, presuming it is explained\n";
+ return 1;
+ }
+ }
+ return 0;
+}
+
sub process {
my $filename = shift;
@@ -2586,6 +2598,7 @@ sub process {
my $has_patch_separator = 0; #Found a --- line
my $has_commit_log = 0; #Encountered lines before patch
my $commit_log_lines = 0; #Number of commit log lines
+ my $eopreamble = 0; # above truncates at =~ /^\. \w+/
my $commit_log_possible_stack_dump = 0;
my $commit_log_long_line = 0;
my $commit_log_has_diff = 0;
@@ -2731,6 +2744,7 @@ sub process {
($line =~ /^rename (?:from|to) \S+\s*$/ ||
$line =~ /^diff --git a\/[\w\/\.\_\-]+ b\/\S+\s*$/))) {
$is_patch = 1;
+ $eopreamble = $linenr;
}
#extract the line range in the file after the patch is applied
@@ -4654,7 +4668,7 @@ sub process {
}
# avoid BUG() or BUG_ON()
- if ($line =~ /\b(?:BUG|BUG_ON)\b/) {
+ if ($line =~ /\b(BUG|BUG_ON)\b/ && !commitmsg_refers_to($1, $eopreamble)) {
my $msg_level = \&WARN;
$msg_level = \&CHK if ($file);
&{$msg_level}("AVOID_BUG",
--
2.31.1
On Fri, 2021-06-25 at 21:40 -0600, Jim Cromie wrote:
> The extern-in-C rule has one important exception: the symbol is
> defined in/by the linker script. By convention, these almost always
> contain: _start, _stop, _end. Suppress the warning on such symbols.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> @@ -6910,7 +6910,8 @@ sub process {
> ? $stat =~ /^.\s*extern\s+/)
> ? {
> ? WARN("AVOID_EXTERNS",
> - "externs should be avoided in .c files\n" . $herecurr);
> + "externs should be avoided in .c files\n($stat)\n" . $herecurr)
> + unless $stat =~ /_start|_stop|_end/;
nak.
As far as I can tell, there's no reason these symbols
should not be in .h files.
besides that:
output is single line, $stat should not be used and
using unless is not desired.
On Sat, Jun 26, 2021 at 12:46 PM Joe Perches <[email protected]> wrote:
>
> On Fri, 2021-06-25 at 21:40 -0600, Jim Cromie wrote:
> > The extern-in-C rule has one important exception: the symbol is
> > defined in/by the linker script. By convention, these almost always
> > contain: _start, _stop, _end. Suppress the warning on such symbols.
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > []
> > @@ -6910,7 +6910,8 @@ sub process {
> > $stat =~ /^.\s*extern\s+/)
> > {
> > WARN("AVOID_EXTERNS",
> > - "externs should be avoided in .c files\n" . $herecurr);
> > + "externs should be avoided in .c files\n($stat)\n" . $herecurr)
> > + unless $stat =~ /_start|_stop|_end/;
>
> nak.
>
> As far as I can tell, there's no reason these symbols
> should not be in .h files.
>
judging from the codebase, it has been a case-by-case decision,
with 8/10 of the linker-vars extern'd into C files, not headers.
[jimc@frodo wk-test]$ perl -ne '/(\w*_(?:start|stop|end)(?:_\w+))/ and
print "$1\n"' `find . -name \*.lds.h` > symbols
[jimc@frodo wk-test]$ wc symbols
99 99 2112 symbols
[jimc@frodo wk-test]$ grep -n -r --exclude-dir=builds/ -f symbols . |
grep -E '\.c:' | grep extern | wc
79 331 6402
[jimc@frodo wk-test]$ grep -n -r --exclude-dir=builds/ -f symbols . |
grep -E '\.h' | grep extern | wc
19 81 1581
8/10 cases dont expose these symbols in headers,
Makes sense to me, mostly theyre internal, and often double-underscored too.
2/10 are presumably in headers for specific reasons.
> besides that:
>
> output is single line, $stat should not be used and
> using unless is not desired.
>
could you clarify ?
style issues are easy, std if form...
$stat is already used, it must contain extern to get here.
checking it for a likely-linker-symbol seems fair.
On Fri, 2021-06-25 at 21:40 -0600, Jim Cromie wrote:
> Before issuing a WARNING on spacing, exclude reports on linker
> scripts, which don't comport to C style rules. skip_on_filename()
> skips on *.lds.h files, I think covering all cases.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -2546,6 +2546,11 @@ sub get_raw_comment {
> ? return $comment;
> ?}
>
> +sub skip_on_filename {
> + my $fname = shift;
> + return $fname =~ m@\.lds\.h$@;
> +}
nak. This is poor naming for what is not a generic function.
> @@ -5134,7 +5139,8 @@ sub process {
> ? }
> ? }
> ? } elsif ($ctx =~ /Wx[^WCE]|[^WCE]xW/) {
> - if (ERROR("SPACING",
> + if (!skip_on_filename($realfile) &&
> + ERROR("SPACING",
> ? "need consistent spacing around '$op' $at\n" . $hereptr)) {
> ? $good = rtrim($fix_elements[$n]) . " " . trim($fix_elements[$n + 1]) . " ";
> ? if (defined $fix_elements[$n + 2]) {
This .lds.h test is also used in one other place.
It might be better to avoid all tests in .lds.h files by using a
"next if" much earlier.
Yes, I agree.
On Sun, Jun 27, 2021 at 9:17 PM Joe Perches <[email protected]> wrote:
>
> On Fri, 2021-06-25 at 21:40 -0600, Jim Cromie wrote:
> > Before issuing a WARNING on spacing, exclude reports on linker
> > scripts, which don't comport to C style rules. skip_on_filename()
> > skips on *.lds.h files, I think covering all cases.
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -2546,6 +2546,11 @@ sub get_raw_comment {
> > return $comment;
> > }
> >
> > +sub skip_on_filename {
> > + my $fname = shift;
> > + return $fname =~ m@\.lds\.h$@;
> > +}
>
> nak. This is poor naming for what is not a generic function.
indeed.
>
> > @@ -5134,7 +5139,8 @@ sub process {
> > }
> > }
> > } elsif ($ctx =~ /Wx[^WCE]|[^WCE]xW/) {
> > - if (ERROR("SPACING",
> > + if (!skip_on_filename($realfile) &&
> > + ERROR("SPACING",
> > "need consistent spacing around '$op' $at\n" . $hereptr)) {
> > $good = rtrim($fix_elements[$n]) . " " . trim($fix_elements[$n + 1]) . " ";
> > if (defined $fix_elements[$n + 2]) {
>
> This .lds.h test is also used in one other place.
>
> It might be better to avoid all tests in .lds.h files by using a
> "next if" much earlier.
YES. I see the code uses 'next if' syntax. so this would work.
next if ($filename =~ m@\.lds\.h$@)
I will find a good spot for this line.
thanks
Jim
hi Joe,
> >
> > This .lds.h test is also used in one other place.
> >
> > It might be better to avoid all tests in .lds.h files by using a
> > "next if" much earlier.
>
checkpatch: subtle decrufting
sub process() uses a next-if statement to end a block of tests early,
because following tests pertain only to certain types of source files.
That statement has some history:
$ grep -P 'sub process|next if \(\$realfile' blame-check
0a920b5b666d0 (Andy Whitcroft 2007-06-01 00:46:48 -0700 2558)
sub process {
d6430f71805aa (Joe Perches 2016-12-12 16:46:28 -0800 3621)
next if ($realfile !~ /\.(h|c|s|S|sh|dtsi|dts)$/);
de4c924c26504 (Geert Uytterhoeven 2014-10-13 15:51:46 -0700 3712)
next if ($realfile !~ /\.(h|c|pl|dtsi|dts)$/);
b9ea10d691ecb (Andy Whitcroft 2008-10-15 22:02:24 -0700 3973)
next if ($realfile !~ /\.(h|c)$/);
Commit:b9ea adds the early-block-termination line, then 2 subsequent
commits (de4c, d643) copy that line up/earlier in sub process (with
different filetype selection), largely masking the purposes of the
older/later lines (block-early-terminate to skip file-type specific
tests).
This code is hurting my brain.
its the combo of
3 different multiple file-type selections, each stricter than previous
early block terminate semantics
!~ (one more logical inversion)
commit:de4c allows further testing on *.pl files
but commit:d643 doesnt allow those same files to reach that spot
ISTM this wasnt quite intended.
The 3rd/original usage still has some effect,
forex when a dts file reaches that line
How to resolve these issues ?
changing d643 to allow *.pl to fall thru for further testing
is probably the best small move.
FWIW, one version of a 1-line fix for *.lds.h files.
this one adds the new line after the 1st of the 3 blame-lines.
Maybe it should be added after the SPDX check (which would complain)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 6469870837d0..1655d88fe5e3 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3633,6 +3633,7 @@ sub process {
# check we are in a valid source file if not then ignore this hunk
next if ($realfile !~ /\.(h|c|s|S|sh|dtsi|dts)$/);
+ next if ($realfile =~ /\.lds\.h$/); # linker scripts
arent real *.h
# check for using SPDX-License-Identifier on the wrong line number
if ($realline != $checklicenseline &&
On Tue, 2021-06-29 at 10:48 -0600, [email protected] wrote:
> hi Joe,
hey Jim.
> > > This .lds.h test is also used in one other place.
> > >
> > > It might be better to avoid all tests in .lds.h files by using a
> > > "next if" much earlier.
>
> checkpatch: subtle decrufting
>
> sub process() uses a next-if statement to end a block of tests early,
> because following tests pertain only to certain types of source files.
> That statement has some history:
>
> ?$ grep -P 'sub process|next if \(\$realfile' blame-check
> ?0a920b5b666d0 (Andy Whitcroft 2007-06-01 00:46:48 -0700 2558) sub process {
> ?d6430f71805aa (Joe Perches 2016-12-12 16:46:28 -0800 3621) next if ($realfile !~ /\.(h|c|s|S|sh|dtsi|dts)$/);
> ?de4c924c26504 (Geert Uytterhoeven 2014-10-13 15:51:46 -0700 3712) next if ($realfile !~ /\.(h|c|pl|dtsi|dts)$/);
Looks like I should have also removed the |pl from this block
when I removed it from commit d6430f71805aa.
Oh well, no real harm done...
> ?b9ea10d691ecb (Andy Whitcroft 2008-10-15 22:02:24 -0700 3973) next if ($realfile !~ /\.(h|c)$/);
>
> Commit:b9ea adds the early-block-termination line, then 2 subsequent
> commits (de4c, d643) copy that line up/earlier in sub process (with
> different filetype selection), largely masking the purposes of the
> older/later lines (block-early-terminate to skip file-type specific
> tests).
Not really.
The first in file order next-if commit d6430f71805aa was a
modification of the earlier commits listed below:
5368df20fb364e
00df344fd06fd6
0a920b5b666d0b
All of these were just additions of various file types to the test.
> This code is hurting my brain.
Perhaps Advil or another leaded or unleaded beverage might help.
They help me...
> changing d643 to allow *.pl to fall thru for further testing
> is probably the best small move.
Definitely not as it's there specifically to avoid long line tests in perl.
> FWIW, one version of a 1-line fix for *.lds.h files.
> this one adds the new line after the 1st of the 3 blame-lines.
> Maybe it should be added after the SPDX check (which would complain)
Maybe a slight reworking of all the "next if" tests would work.
I moved the incorrect spdx line number test up, but didn't test
whether or not it's appropriate here as I don't know of a case
of the top of my head. I also don't know if the linker .lds.h
files should be tested for long lines or not.
It looks like these files are mostly < 80 columns
$ git ls-files -- '*.lds.h'| xargs cat | awk '{print length($0), $0;}' | sort -rn | head
106 #define DATA_MAIN .data .data.[0-9a-zA-Z_]* .data..L* .data..compoundliteral* .data.$__unnamed_* .data.$L*
94 #if defined(CONFIG_GCOV_KERNEL) || defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KCSAN) || \
79 /* Alignment must be consistent with (kunit_suite *) in include/kunit/test.h */
78 * [__init_begin, __init_end] is the init section that may be freed after init
78 #if defined(CONFIG_LD_DEAD_CODE_DATA_ELIMINATION) || defined(CONFIG_LTO_CLANG)
77 * .init section and thus will be preserved for later use in the decompressed
77 #define RESERVEDMEM_OF_TABLES() OF_TABLE(CONFIG_OF_RESERVED_MEM, reservedmem)
77 * <asm/module.lds.h> can specify arch-specific sections for linking modules.
76 #define CPUIDLE_METHOD_OF_TABLES() OF_TABLE(CONFIG_CPU_IDLE, cpuidle_method)
76 * .boot.data variables are kept in separate .boot.data.<var name> sections,
---
scripts/checkpatch.pl | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 461d4221e4a4a..ea198499e16df 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3617,9 +3617,6 @@ sub process {
"It's generally not useful to have the filename in the file\n" . $herecurr);
}
-# check we are in a valid source file if not then ignore this hunk
- next if ($realfile !~ /\.(h|c|s|S|sh|dtsi|dts)$/);
-
# check for using SPDX-License-Identifier on the wrong line number
if ($realline != $checklicenseline &&
$rawline =~ /\bSPDX-License-Identifier:/ &&
@@ -3628,6 +3625,9 @@ sub process {
"Misplaced SPDX-License-Identifier tag - use line $checklicenseline instead\n" . $herecurr);
}
+# check we are in a valid source file if not then ignore this hunk
+ next if ($realfile !~ /\.(?:h|c|s|S|sh|dtsi|dts)$/);
+
# line length limit (with some exclusions)
#
# There are a few types of lines that may extend beyond $max_line_length:
@@ -3708,8 +3708,8 @@ sub process {
"Avoid using '.L' prefixed local symbol names for denoting a range of code via 'SYM_*_START/END' annotations; see Documentation/asm-annotations.rst\n" . $herecurr);
}
-# check we are in a valid source file C or perl if not then ignore this hunk
- next if ($realfile !~ /\.(h|c|pl|dtsi|dts)$/);
+# check we are in a valid source C or .dts? file, if not then ignore this hunk
+ next if ($realfile !~ /\.(?:h|c|dtsi|dts)$/);
# at the beginning of a line any tabs must come first and anything
# more than $tabsize must use tabs.
@@ -3737,6 +3737,9 @@ sub process {
}
}
+# skip all following test for linker files.
+ next if ($realfile =~ /\.lds\.h$/);
+
# check for assignments on the start of a line
if ($sline =~ /^\+\s+($Assignment)[^=]/) {
my $operator = $1;
@@ -3970,7 +3973,7 @@ sub process {
}
# check we are in a valid C source file if not then ignore this hunk
- next if ($realfile !~ /\.(h|c)$/);
+ next if ($realfile !~ /\.(?:h|c)$/);
# check for unusual line ending [ or (
if ($line =~ /^\+.*([\[\(])\s*$/) {
On Tue, Jun 29, 2021 at 12:28 PM Joe Perches <[email protected]> wrote:
>
> On Tue, 2021-06-29 at 10:48 -0600, [email protected] wrote:
> > hi Joe,
>
> hey Jim.
>
> > > > This .lds.h test is also used in one other place.
> > > >
> > > > It might be better to avoid all tests in .lds.h files by using a
> > > > "next if" much earlier.
> >
> > checkpatch: subtle decrufting
> >
> > sub process() uses a next-if statement to end a block of tests early,
> > because following tests pertain only to certain types of source files.
> > That statement has some history:
> >
> > $ grep -P 'sub process|next if \(\$realfile' blame-check
> > 0a920b5b666d0 (Andy Whitcroft 2007-06-01 00:46:48 -0700 2558) sub process {
> > d6430f71805aa (Joe Perches 2016-12-12 16:46:28 -0800 3621) next if ($realfile !~ /\.(h|c|s|S|sh|dtsi|dts)$/);
> > de4c924c26504 (Geert Uytterhoeven 2014-10-13 15:51:46 -0700 3712) next if ($realfile !~ /\.(h|c|pl|dtsi|dts)$/);
>
> Looks like I should have also removed the |pl from this block
> when I removed it from commit d6430f71805aa.
>
> Oh well, no real harm done...
>
> > b9ea10d691ecb (Andy Whitcroft 2008-10-15 22:02:24 -0700 3973) next if ($realfile !~ /\.(h|c)$/);
> >
> > Commit:b9ea adds the early-block-termination line, then 2 subsequent
> > commits (de4c, d643) copy that line up/earlier in sub process (with
> > different filetype selection), largely masking the purposes of the
> > older/later lines (block-early-terminate to skip file-type specific
> > tests).
>
> Not really.
>
> The first in file order next-if commit d6430f71805aa was a
> modification of the earlier commits listed below:
>
> 5368df20fb364e
> 00df344fd06fd6
> 0a920b5b666d0b
>
> All of these were just additions of various file types to the test.
>
> > This code is hurting my brain.
>
> Perhaps Advil or another leaded or unleaded beverage might help.
> They help me...
>
> > changing d643 to allow *.pl to fall thru for further testing
> > is probably the best small move.
>
> Definitely not as it's there specifically to avoid long line tests in perl.
>
Ok, I see that.. its all pretty subtle.
> > FWIW, one version of a 1-line fix for *.lds.h files.
> > this one adds the new line after the 1st of the 3 blame-lines.
> > Maybe it should be added after the SPDX check (which would complain)
>
I tried that 1-liner : next if lds.h
after each of those 3 blame-lines, the latter 2 have effect
(subtracting errors)
the first doesnt improve on the later one.
[jimc@frodo wk-other]$ git log
commit 6825b43bae4cd51808050de3769f1d3df5b0bc76 (HEAD -> checkpatch-2)
Author: Jim Cromie <[email protected]>
Date: Tue Jun 29 12:34:40 2021 -0600
checkpatch: skip more tests on linker scripts
before:
total: 1 errors, 7 warnings, 1152 lines checked
after:
total: 0 errors, 1 warnings, 1152 lines checked
commit a9f9a26a299097f9b6524a25681cc45c04aec6b5
Author: Jim Cromie <[email protected]>
Date: Tue Jun 29 12:31:29 2021 -0600
checkpatch: skip (c|h) tests on linker scripts
testing with:
$ scripts/checkpatch.pl -f include/asm-generic/vmlinux.lds.h
before:
total: 18 errors, 8 warnings, 1152 lines checked
after:
total: 1 errors, 7 warnings, 1152 lines checked
> Maybe a slight reworking of all the "next if" tests would work.
>
> I moved the incorrect spdx line number test up, but didn't test
> whether or not it's appropriate here as I don't know of a case
> of the top of my head. I also don't know if the linker .lds.h
> files should be tested for long lines or not.
ISTM the -f option is that test-case - use existing file ( vmlinux.lds.h )
as that reference, since it exists, everything in it is "approved style"
The current reference elicits 18 errors and 8 warnings,
ISTM those tests are inappropriate for this kind of file.
>
> It looks like these files are mostly < 80 columns
>
> $ git ls-files -- '*.lds.h'| xargs cat | awk '{print length($0), $0;}' | sort -rn | head
> 106 #define DATA_MAIN .data .data.[0-9a-zA-Z_]* .data..L* .data..compoundliteral* .data.$__unnamed_* .data.$L*
> 94 #if defined(CONFIG_GCOV_KERNEL) || defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KCSAN) || \
> 79 /* Alignment must be consistent with (kunit_suite *) in include/kunit/test.h */
> 78 * [__init_begin, __init_end] is the init section that may be freed after init
> 78 #if defined(CONFIG_LD_DEAD_CODE_DATA_ELIMINATION) || defined(CONFIG_LTO_CLANG)
> 77 * .init section and thus will be preserved for later use in the decompressed
> 77 #define RESERVEDMEM_OF_TABLES() OF_TABLE(CONFIG_OF_RESERVED_MEM, reservedmem)
> 77 * <asm/module.lds.h> can specify arch-specific sections for linking modules.
> 76 #define CPUIDLE_METHOD_OF_TABLES() OF_TABLE(CONFIG_CPU_IDLE, cpuidle_method)
> 76 * .boot.data variables are kept in separate .boot.data.<var name> sections,
>
> ---
> scripts/checkpatch.pl | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
this does 3 different things
- non-capturing matches - these add no functionality,
- moves the skip-remaining-tests check after SPDX
that feels like a legal Q: should it be on all files ?
moving it does seem proper though.
- adds the skip linker-script
since i went ahead and added it 3 times to see errs/warns
I didnt consider your precise placement,
how does it do with 18/8 errs/warnings on ref-test ?
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 461d4221e4a4a..ea198499e16df 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3617,9 +3617,6 @@ sub process {
> "It's generally not useful to have the filename in the file\n" . $herecurr);
> }
>
> -# check we are in a valid source file if not then ignore this hunk
> - next if ($realfile !~ /\.(h|c|s|S|sh|dtsi|dts)$/);
> -
> # check for using SPDX-License-Identifier on the wrong line number
> if ($realline != $checklicenseline &&
> $rawline =~ /\bSPDX-License-Identifier:/ &&
> @@ -3628,6 +3625,9 @@ sub process {
> "Misplaced SPDX-License-Identifier tag - use line $checklicenseline instead\n" . $herecurr);
> }
>
> +# check we are in a valid source file if not then ignore this hunk
> + next if ($realfile !~ /\.(?:h|c|s|S|sh|dtsi|dts)$/);
> +
> # line length limit (with some exclusions)
> #
> # There are a few types of lines that may extend beyond $max_line_length:
> @@ -3708,8 +3708,8 @@ sub process {
> "Avoid using '.L' prefixed local symbol names for denoting a range of code via 'SYM_*_START/END' annotations; see Documentation/asm-annotations.rst\n" . $herecurr);
> }
>
> -# check we are in a valid source file C or perl if not then ignore this hunk
> - next if ($realfile !~ /\.(h|c|pl|dtsi|dts)$/);
> +# check we are in a valid source C or .dts? file, if not then ignore this hunk
> + next if ($realfile !~ /\.(?:h|c|dtsi|dts)$/);
>
> # at the beginning of a line any tabs must come first and anything
> # more than $tabsize must use tabs.
> @@ -3737,6 +3737,9 @@ sub process {
> }
> }
>
> +# skip all following test for linker files.
> + next if ($realfile =~ /\.lds\.h$/);
> +
> # check for assignments on the start of a line
> if ($sline =~ /^\+\s+($Assignment)[^=]/) {
> my $operator = $1;
> @@ -3970,7 +3973,7 @@ sub process {
> }
>
> # check we are in a valid C source file if not then ignore this hunk
> - next if ($realfile !~ /\.(h|c)$/);
> + next if ($realfile !~ /\.(?:h|c)$/);
>
> # check for unusual line ending [ or (
> if ($line =~ /^\+.*([\[\(])\s*$/) {
>
On Tue, 2021-06-29 at 13:50 -0600, [email protected] wrote:
> this does 3 different things
>
> - non-capturing matches - these add no functionality,
true, it's nominally a bit faster through.
> - moves the skip-remaining-tests check after SPDX
> ???that feels like a legal Q: should it be on all files ?
> ???moving it does seem proper though.
to me too.
> - adds the skip linker-script
> ??since i went ahead and added it 3 times to see errs/warns
> ??I didnt consider your precise placement,
> ??how does it do with 18/8 errs/warnings on ref-test ?
$ ./scripts/checkpatch.pl -f include/asm-generic/vmlinux.lds.h --strict --terse
include/asm-generic/vmlinux.lds.h:1: WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
include/asm-generic/vmlinux.lds.h:43: WARNING: please, no space before tabs
include/asm-generic/vmlinux.lds.h:101: CHECK: line length of 106 exceeds 100 columns
include/asm-generic/vmlinux.lds.h:390: WARNING: please, no space before tabs
include/asm-generic/vmlinux.lds.h:546: ERROR: code indent should use tabs where possible
total: 1 errors, 3 warnings, 1 checks, 1184 lines checked
On Tue, Jun 29, 2021 at 2:01 PM Joe Perches <[email protected]> wrote:
>
> On Tue, 2021-06-29 at 13:50 -0600, [email protected] wrote:
> > this does 3 different things
> >
> > - non-capturing matches - these add no functionality,
>
> true, it's nominally a bit faster through.
>
> > - moves the skip-remaining-tests check after SPDX
> > that feels like a legal Q: should it be on all files ?
> > moving it does seem proper though.
>
> to me too.
>
> > - adds the skip linker-script
> > since i went ahead and added it 3 times to see errs/warns
> > I didnt consider your precise placement,
> > how does it do with 18/8 errs/warnings on ref-test ?
>
> $ ./scripts/checkpatch.pl -f include/asm-generic/vmlinux.lds.h --strict --terse
cool options.
<Aside>
some oddities are hidden there;
Im seeing the err/warn counts change along with use of those options.
not a big deal, but it is mildly surprising
forex:
$ scripts/checkpatch.pl -f include/asm-generic/vmlinux.lds.h --terse
...
total: 18 errors, 7 warnings, 1164 lines checked
$ scripts/checkpatch.pl -f include/asm-generic/vmlinux.lds.h --terse --strict
...
total: 9 errors, 7 warnings, 95 checks, 1164 lines checked
> include/asm-generic/vmlinux.lds.h:1: WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
> include/asm-generic/vmlinux.lds.h:43: WARNING: please, no space before tabs
> include/asm-generic/vmlinux.lds.h:101: CHECK: line length of 106 exceeds 100 columns
> include/asm-generic/vmlinux.lds.h:390: WARNING: please, no space before tabs
> include/asm-generic/vmlinux.lds.h:546: ERROR: code indent should use tabs where possible
> total: 1 errors, 3 warnings, 1 checks, 1184 lines checked
>
2nd one is I think pedantic comment formatting, but on the whole, fair
complaints.
and I see your insertion spot is right between my 2 picks.
works for me.
just to note, this is about a generalization of
commit 263afd39c06f5939ef943e0d535380d4b8e56484
Author: Chris Down <[email protected]>
Date: Thu Feb 25 17:22:04 2021 -0800
checkpatch: don't warn about colon termination in linker scripts
On Wed, 2021-06-30 at 10:38 -0600, [email protected] wrote:
> On Tue, Jun 29, 2021 at 2:01 PM Joe Perches <[email protected]> wrote:
> >
> > On Tue, 2021-06-29 at 13:50 -0600, [email protected] wrote:
> > > this does 3 different things
> > >
> > > - non-capturing matches - these add no functionality,
> >
> > true, it's nominally a bit faster through.
> >
> > > - moves the skip-remaining-tests check after SPDX
> > > ???that feels like a legal Q: should it be on all files ?
> > > ???moving it does seem proper though.
> >
> > to me too.
> >
> > > - adds the skip linker-script
> > > ??since i went ahead and added it 3 times to see errs/warns
> > > ??I didnt consider your precise placement,
> > > ??how does it do with 18/8 errs/warnings on ref-test ?
> >
> > $ ./scripts/checkpatch.pl -f include/asm-generic/vmlinux.lds.h --strict --terse
>
> cool options.
> <Aside>
> some oddities are hidden there;
> Im seeing the err/warn counts change along with use of those options.
> not a big deal, but it is mildly surprising
> forex:
> $ scripts/checkpatch.pl -f include/asm-generic/vmlinux.lds.h --terse
> ...
> total: 18 errors, 7 warnings, 1164 lines checked
> $ scripts/checkpatch.pl -f include/asm-generic/vmlinux.lds.h --terse --strict
> ...
> total: 9 errors, 7 warnings, 95 checks, 1164 lines checked
The difference is a --strict test 'if ($check)' that precedes and
in effect 'overrides' the ERROR output for that output around line 5120.
$ ./scripts/checkpatch.pl -f include/asm-generic/vmlinux.lds.h --terse
[]
include/asm-generic/vmlinux.lds.h:101: ERROR: need consistent spacing around '*' (ctx:VxW)
include/asm-generic/vmlinux.lds.h:101: ERROR: need consistent spacing around '*' (ctx:VxW)
include/asm-generic/vmlinux.lds.h:101: ERROR: need consistent spacing around '*' (ctx:VxW)
include/asm-generic/vmlinux.lds.h:101: ERROR: need consistent spacing around '*' (ctx:VxW)
vs:
$ ./scripts/checkpatch.pl -f include/asm-generic/vmlinux.lds.h --terse --strict
[]
include/asm-generic/vmlinux.lds.h:101: CHECK: spaces preferred around that '*' (ctx:VxW)
include/asm-generic/vmlinux.lds.h:101: CHECK: spaces preferred around that '*' (ctx:VxW)
include/asm-generic/vmlinux.lds.h:101: CHECK: spaces preferred around that '*' (ctx:VxW)
include/asm-generic/vmlinux.lds.h:101: CHECK: spaces preferred around that '*' (ctx:VxW)
> just to note, this is about a generalization of
>
> commit 263afd39c06f5939ef943e0d535380d4b8e56484
> Author: Chris Down <[email protected]>
> Date: Thu Feb 25 17:22:04 2021 -0800
>
> ????checkpatch: don't warn about colon termination in linker scripts
Which means the additional test in that commit should be removed too.
Maybe:
---
scripts/checkpatch.pl | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 461d4221e4a4a..f4f5826054214 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3617,9 +3617,6 @@ sub process {
"It's generally not useful to have the filename in the file\n" . $herecurr);
}
-# check we are in a valid source file if not then ignore this hunk
- next if ($realfile !~ /\.(h|c|s|S|sh|dtsi|dts)$/);
-
# check for using SPDX-License-Identifier on the wrong line number
if ($realline != $checklicenseline &&
$rawline =~ /\bSPDX-License-Identifier:/ &&
@@ -3628,6 +3625,9 @@ sub process {
"Misplaced SPDX-License-Identifier tag - use line $checklicenseline instead\n" . $herecurr);
}
+# check we are in a valid source file if not then ignore this hunk
+ next if ($realfile !~ /\.(?:h|c|s|S|sh|dtsi|dts)$/);
+
# line length limit (with some exclusions)
#
# There are a few types of lines that may extend beyond $max_line_length:
@@ -3708,8 +3708,8 @@ sub process {
"Avoid using '.L' prefixed local symbol names for denoting a range of code via 'SYM_*_START/END' annotations; see Documentation/asm-annotations.rst\n" . $herecurr);
}
-# check we are in a valid source file C or perl if not then ignore this hunk
- next if ($realfile !~ /\.(h|c|pl|dtsi|dts)$/);
+# check we are in a valid source C or .dts? file, if not then ignore this hunk
+ next if ($realfile !~ /\.(?:h|c|dtsi|dts)$/);
# at the beginning of a line any tabs must come first and anything
# more than $tabsize must use tabs.
@@ -3737,6 +3737,9 @@ sub process {
}
}
+# skip all following test for linker files.
+ next if ($realfile =~ /\.lds\.h$/);
+
# check for assignments on the start of a line
if ($sline =~ /^\+\s+($Assignment)[^=]/) {
my $operator = $1;
@@ -3970,7 +3973,7 @@ sub process {
}
# check we are in a valid C source file if not then ignore this hunk
- next if ($realfile !~ /\.(h|c)$/);
+ next if ($realfile !~ /\.(?:h|c)$/);
# check for unusual line ending [ or (
if ($line =~ /^\+.*([\[\(])\s*$/) {
@@ -5147,7 +5150,7 @@ sub process {
# A colon needs no spaces before when it is
# terminating a case value or a label.
} elsif ($opv eq ':C' || $opv eq ':L') {
- if ($ctx =~ /Wx./ and $realfile !~ m@.*\.lds\.h$@) {
+ if ($ctx =~ /Wx./) {
if (ERROR("SPACING",
"space prohibited before that '$op' $at\n" . $hereptr)) {
$good = rtrim($fix_elements[$n]) . trim($fix_elements[$n + 1]);
On Sat, 2021-06-26 at 21:47 -0600, [email protected] wrote:
> On Sat, Jun 26, 2021 at 12:46 PM Joe Perches <[email protected]> wrote:
> > On Fri, 2021-06-25 at 21:40 -0600, Jim Cromie wrote:
> > > The extern-in-C rule has one important exception: the symbol is
> > > defined in/by the linker script. By convention, these almost always
> > > contain: _start, _stop, _end. Suppress the warning on such symbols.
> > []
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > []
> > > @@ -6910,7 +6910,8 @@ sub process {
> > > ??????????????????$stat =~ /^.\s*extern\s+/)
> > > ??????????????{
> > > ??????????????????????WARN("AVOID_EXTERNS",
> > > - "externs should be avoided in .c files\n" . $herecurr);
> > > + "externs should be avoided in .c files\n($stat)\n" . $herecurr)
> > > + unless $stat =~ /_start|_stop|_end/;
> >
> > nak.
> >
> > As far as I can tell, there's no reason these symbols
> > should not be in .h files.
>
> judging from the codebase, it has been a case-by-case decision,
> with 8/10 of the linker-vars extern'd into C files, not headers.
[]
> > besides that:
> >
> > output is single line, $stat should not be used and
> > using unless is not desired.
> >
>
> could you clarify ?
> style issues are easy, std if form...
> $stat is already used, it must contain extern to get here.
Sure, it's used as part of a test but it's never output as part of
an error message. $stat strips any leading '+' from the 2nd and
subsequent lines.
There's a mechanism used in several other tests to show these lines.
my $cnt = statement_rawlines($stat);
my $herectx = get_stat_here($linenr, $cnt, $here);
with the output of $herectx.
> checking it for a likely-linker-symbol seems fair.