From: Björn Töpel <[email protected]>
Checkpatch sometimes report a false positive for __initconst. E.g., for the
following snippet:
| static const struct strspn_test {
| const char str[16];
| const char accept[16];
| const char reject[16];
| unsigned a;
| unsigned r;
| } tests[] __initconst = {
| { "foobar", "", "", 0, 6 },
| { "abba", "abc", "ABBA", 4, 4 },
| { "abba", "a", "b", 1, 1 },
| { "", "abc", "abc", 0, 0},
| };
checkpatch would report:
| ERROR: Use of __initconst requires a separate use of const
| #190: FILE: ./test_string.c:190:
| + } tests[] __initconst = {
Improve the reporting by trying harder to find the 'const'.
Signed-off-by: Björn Töpel <[email protected]>
---
In [1], Andy asked if it was possible to fix the __initconst false
positive in checkpatch, rather than the code.
I did a crude hack that searches backwards for the 'const' in the
struct definition, but I'm sure the Perl hackers out there hate it,
hence the RFC. ;-)
Björn
[1] https://lore.kernel.org/linux-riscv/CAHp75VfK3RM+SP90d3nOXEobY81Xd_94tLL=Qt86mmdNwXaQpg@mail.gmail.com/
---
scripts/checkpatch.pl | 54 ++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 53 insertions(+), 1 deletion(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index bd44d12965c9..d2370233e2c1 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1854,6 +1854,48 @@ sub ctx_statement_full {
return ($level, $linenr, @chunks);
}
+sub ctx_block_outer_rev {
+ my ($linenr, $open, $close) = @_;
+ my $line;
+ my $start = $linenr;
+ my $blk = '';
+ my @res = ();
+
+ my $level = 0;
+ my @stack = ($level);
+ for ($line = $start; $line >= 0; $line--) {
+ next if ($rawlines[$line] =~ /^-/);
+
+ $blk .= $rawlines[$line];
+
+ # Handle nested #if/#else.
+ if ($lines[$line] =~ /^.\s*#\s*(?:ifndef|ifdef|if)\s/) {
+ $level = pop(@stack);
+ } elsif ($lines[$line] =~ /^.\s*#\s*(?:else|elif)\b/) {
+ $level = $stack[$#stack - 1];
+ } elsif ($lines[$line] =~ /^.\s*#\s*endif\b/) {
+ push(@stack, $level);
+ }
+
+ foreach my $c (split(//, $lines[$line])) {
+ if ($c eq $close && $level > 0) {
+ $level--;
+ last if ($level == 0);
+ } elsif ($c eq $open) {
+ $level++;
+ }
+ }
+
+ if ($level <= 1) {
+ push(@res, $rawlines[$line]);
+ }
+
+ last if ($level == 0);
+ }
+
+ return @res;
+}
+
sub ctx_block_get {
my ($linenr, $remain, $outer, $open, $close, $off) = @_;
my $line;
@@ -6502,7 +6544,17 @@ sub process {
# check for $InitAttributeConst (ie: __initconst) without const
if ($line !~ /\bconst\b/ && $line =~ /($InitAttributeConst)/) {
my $attr = $1;
- if (ERROR("INIT_ATTRIBUTE",
+ my $error = 1;
+ if ($line =~ /}/) {
+ # The const might be part of a struct definition. Try to find that...
+ my @ctx = ctx_block_outer_rev($linenr, '}', '{');
+ if (@ctx) {
+ if ($ctx[$#ctx] =~ /\bconst\b/) {
+ $error = 0;
+ }
+ }
+ }
+ if ($error && ERROR("INIT_ATTRIBUTE",
"Use of $attr requires a separate use of const\n" . $herecurr) &&
$fix) {
my $lead = $fixed[$fixlinenr] =~
base-commit: c0927a7a5391f7d8e593e5e50ead7505a23cadf9
--
2.37.2
On Wed, Mar 01, 2023 at 10:43:20AM +0100, Bj?rn T?pel wrote:
> From: Bj?rn T?pel <[email protected]>
>
> Checkpatch sometimes report a false positive for __initconst. E.g., for the
> following snippet:
>
> | static const struct strspn_test {
> | const char str[16];
> | const char accept[16];
> | const char reject[16];
> | unsigned a;
> | unsigned r;
> | } tests[] __initconst = {
> | { "foobar", "", "", 0, 6 },
> | { "abba", "abc", "ABBA", 4, 4 },
> | { "abba", "a", "b", 1, 1 },
> | { "", "abc", "abc", 0, 0},
> | };
>
> checkpatch would report:
>
> | ERROR: Use of __initconst requires a separate use of const
> | #190: FILE: ./test_string.c:190:
> | + } tests[] __initconst = {
>
> Improve the reporting by trying harder to find the 'const'.
Joe, what do you think about this?
--
With Best Regards,
Andy Shevchenko
On Tue, 2023-04-11 at 16:38 +0300, Andy Shevchenko wrote:
> On Wed, Mar 01, 2023 at 10:43:20AM +0100, Bj?rn T?pel wrote:
> > From: Bj?rn T?pel <[email protected]>
> >
> > Checkpatch sometimes report a false positive for __initconst. E.g., for the
> > following snippet:
> >
> > | static const struct strspn_test {
> > | const char str[16];
> > | const char accept[16];
> > | const char reject[16];
> > | unsigned a;
> > | unsigned r;
> > | } tests[] __initconst = {
> > | { "foobar", "", "", 0, 6 },
> > | { "abba", "abc", "ABBA", 4, 4 },
> > | { "abba", "a", "b", 1, 1 },
> > | { "", "abc", "abc", 0, 0},
> > | };
> >
> > checkpatch would report:
> >
> > | ERROR: Use of __initconst requires a separate use of const
> > | #190: FILE: ./test_string.c:190:
> > | + } tests[] __initconst = {
> >
> > Improve the reporting by trying harder to find the 'const'.
>
> Joe, what do you think about this?
I think the ctx_block_outer_rev function doesn't handle patch
context blocks and the loop at best needs to be changed to include
last if $rawlines[$line] =~ /^@/);.
And the loop parsing couldn't handle structs with embedded
unions or structs.
I also think that checkpatch will always have false negatives
and false positives and this might not be that useful as likely
most compilers should now be able to identify this as well.