Checkpatch reports warnings when some specific structs are not declared
as const in the code. The list of structs to consider was initially
defined in the checkpatch.pl script itself, but it was later moved to an
external file (scripts/const_structs.checkpatch), in commit bf1fa1dae68e
("checkpatch: externalize the structs that should be const"). This
introduced two minor issues:
- When file scripts/const_structs.checkpatch is not present (for
example, if checkpatch is run outside of the kernel directory with the
"--no-tree" option), a warning is printed to stderr to tell the user
that "No structs that should be const will be found". This is fair,
but the warning is printed unconditionally, even if the option
"--ignore CONST_STRUCT" is passed. In the latter case, we explicitly
ask checkpatch to skip this check, so no warning should be printed.
- When scripts/const_structs.checkpatch is missing, or even when trying
to silence the warning by adding an empty file, $const_structs is set
to "", and the regex used for finding structs that should be const,
"$line =~ /\bstruct\s+($const_structs)\b(?!\s*\{)/)", matches all
structs found in the code, thus reporting a number of false positives.
Let's fix the first item by skipping scripts/const_structs.checkpatch
processing if "CONST_STRUCT" checks are ignored, and the second one by
skipping the test if $const_structs is not defined.
Signed-off-by: Quentin Monnet <[email protected]>
---
v2:
- Check if $const_structs is defined instead of non-empty.
- Remove "Fixes" tag.
---
scripts/checkpatch.pl | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index db9d94f90431..3b14bf3e4d4e 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -770,7 +770,7 @@ sub read_words {
next;
}
- $$wordsRef .= '|' if ($$wordsRef ne "");
+ $$wordsRef .= '|' if (defined($$wordsRef) && $$wordsRef ne "");
$$wordsRef .= $line;
}
close($file);
@@ -780,9 +780,11 @@ sub read_words {
return 0;
}
-my $const_structs = "";
-read_words(\$const_structs, $conststructsfile)
- or warn "No structs that should be const will be found - file '$conststructsfile': $!\n";
+my $const_structs;
+if (show_type("CONST_STRUCT")) {
+ read_words(\$const_structs, $conststructsfile)
+ or warn "No structs that should be const will be found - file '$conststructsfile': $!\n";
+}
my $typeOtherTypedefs = "";
if (length($typedefsfile)) {
@@ -6656,7 +6658,8 @@ sub process {
# check for various structs that are normally const (ops, kgdb, device_tree)
# and avoid what seem like struct definitions 'struct foo {'
- if ($line !~ /\bconst\b/ &&
+ if (defined($const_structs) &&
+ $line !~ /\bconst\b/ &&
$line =~ /\bstruct\s+($const_structs)\b(?!\s*\{)/) {
WARN("CONST_STRUCT",
"struct $1 should normally be const\n" . $herecurr);
--
2.20.1
Hi again.
On Tue, 2020-06-23 at 11:37 +0100, Quentin Monnet wrote:
> Checkpatch reports warnings when some specific structs are not declared
> as const in the code. The list of structs to consider was initially
> defined in the checkpatch.pl script itself, but it was later moved to an
> external file (scripts/const_structs.checkpatch), in commit bf1fa1dae68e
> ("checkpatch: externalize the structs that should be const"). This
> introduced two minor issues:
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -770,7 +770,7 @@ sub read_words {
> next;
> }
>
> - $$wordsRef .= '|' if ($$wordsRef ne "");
> + $$wordsRef .= '|' if (defined($$wordsRef) && $$wordsRef ne "");
perl is a weird language and the $$wordsRef ne "" test
isn't required as the append will work even if the
thing being appended to isn't defined.
You can read the perlsyn docs
http://perldoc.perl.org/perlsyn.html
or
https://stackoverflow.com/questions/2166575/when-is-it-ok-to-use-an-undefined-variable-in-perl-with-warnings-enabled
so perhaps remove the test and improve the additional
$typedefsfile use too
---
scripts/checkpatch.pl | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index b06093777fd8..9210267a7771 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -62,7 +62,7 @@ my $spelling_file = "$D/spelling.txt";
my $codespell = 0;
my $codespellfile = "/usr/share/codespell/dictionary.txt";
my $conststructsfile = "$D/const_structs.checkpatch";
-my $typedefsfile = "";
+my $typedefsfile;
my $color = "auto";
my $allow_c99_comments = 1; # Can be overridden by --ignore C99_COMMENT_TOLERANCE
# git output parsing needs US English output, so first set backtick child process LANGUAGE
@@ -770,7 +770,7 @@ sub read_words {
next;
}
- $$wordsRef .= '|' if ($$wordsRef ne "");
+ $$wordsRef .= '|' if (defined($$wordsRef));
$$wordsRef .= $line;
}
close($file);
@@ -780,16 +780,18 @@ sub read_words {
return 0;
}
-my $const_structs = "";
-read_words(\$const_structs, $conststructsfile)
- or warn "No structs that should be const will be found - file '$conststructsfile': $!\n";
+my $const_structs;
+if (show_type("CONST_STRUCT")) {
+ read_words(\$const_structs, $conststructsfile)
+ or warn "No structs that should be const will be found - file '$conststructsfile': $!\n";
-my $typeOtherTypedefs = "";
-if (length($typedefsfile)) {
+
+if (defined($typedefsfile)) {
+ my $typeOtherTypedefs;
read_words(\$typeOtherTypedefs, $typedefsfile)
or warn "No additional types will be considered - file '$typedefsfile': $!\n";
+ $typeTypedefs .= '|' . $typeOtherTypedefs if (defined $typeOtherTypedefs);
}
-$typeTypedefs .= '|' . $typeOtherTypedefs if ($typeOtherTypedefs ne "");
sub build_types {
my $mods = "(?x: \n" . join("|\n ", (@modifierList, @modifierListFile)) . "\n)";
@@ -6660,7 +6662,8 @@ sub process {
# check for various structs that are normally const (ops, kgdb, device_tree)
# and avoid what seem like struct definitions 'struct foo {'
- if ($line !~ /\bconst\b/ &&
+ if (defined($const_structs) &&
+ $line !~ /\bconst\b/ &&
$line =~ /\bstruct\s+($const_structs)\b(?!\s*\{)/) {
WARN("CONST_STRUCT",
"struct $1 should normally be const\n" . $herecurr);
On Tue, 23 Jun 2020 at 17:48, Joe Perches <[email protected]> wrote:
>
> Hi again.
>
> On Tue, 2020-06-23 at 11:37 +0100, Quentin Monnet wrote:
> > Checkpatch reports warnings when some specific structs are not declared
> > as const in the code. The list of structs to consider was initially
> > defined in the checkpatch.pl script itself, but it was later moved to an
> > external file (scripts/const_structs.checkpatch), in commit bf1fa1dae68e
> > ("checkpatch: externalize the structs that should be const"). This
> > introduced two minor issues:
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -770,7 +770,7 @@ sub read_words {
> > next;
> > }
> >
> > - $$wordsRef .= '|' if ($$wordsRef ne "");
> > + $$wordsRef .= '|' if (defined($$wordsRef) && $$wordsRef ne "");
>
> perl is a weird language and the $$wordsRef ne "" test
> isn't required as the append will work even if the
> thing being appended to isn't defined.
>
> You can read the perlsyn docs
> http://perldoc.perl.org/perlsyn.html
> or
> https://stackoverflow.com/questions/2166575/when-is-it-ok-to-use-an-undefined-variable-in-perl-with-warnings-enabled
I didn't want to touch $typeOtherTypedefs in v2, and I thought I
needed that « $$wordsRef ne "" » for the
"read_words(\$typeOtherTypedefs, $typedefsfile)" where the variable
was defined but possibly empty? Anyway, thanks for the lore and the
links :).
>
> so perhaps remove the test and improve the additional
> $typedefsfile use too
Agreed, your version looks better. I'll resubmit a v3 with your
suggestions, please feel free to comment and add a "Co-authored-by:"
tag if you feel this is appropriate.
Thank you,
Quentin