Andy Whitcroft: checkpatch internals question for you below:
On Sat, 2020-04-11 at 08:48 +0200, Christophe JAILLET wrote:
> Le 10/04/2020 à 21:53, Joe Perches a écrit :
> > On Fri, 2020-04-10 at 12:46 -0700, Joe Perches wrote:
> > > On Fri, 2020-04-10 at 19:35 +0200, Christophe JAILLET wrote:
> > > > Le 08/04/2020 à 04:14, Joe Perches a écrit :
> > > > > This works rather better:
> > > > > Perhaps you could test?
> > > []
> > > > I'm looking at some modification done in the last month that could have
> > > > been spotted by the above script.
> > > >
> > > > ./scripts/checkpatch.pl -f drivers/usb/phy/phy-jz4770.c
> > > >
> > > > correctly spots the 3 first cases, but the 3 last (line 202, 210 and
> > > > 217) are missed.
> > > > I don't understand why.
> > > It has to do with checkpatch's single statement parsing.
> > >
> > > This case:
> > >
> > > if (foo)
> > > dev_warn(...);
> > >
> > > is parsed as a single statement but
> > >
> > > if (foo) {
> > > dev_warn(...);
> > > };
> > >
> > > is parsed as multiple statements so for the
> > > second case
> > >
> > > dev_warn(...);
> > >
> > > is analyzed as a separate statement.
> > >
> > > The regex match for this missing newline test expects
> > > that each printk is a separate statement so the first
> > > case doesn't match.
> > >
> > > Clearly the regex can be improved here.
> > So on top of the original patch:
> > ---
> > scripts/checkpatch.pl | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index f00a6c8..54eaa7 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -5675,8 +5675,8 @@ sub process {
> >
> > # check for possible missing newlines at the end of common logging functions
> > if (defined($stat) &&
> > - $stat =~ /^\+\s*($logFunctions)\s*\((?:\s*$FuncArg\s*,\s*){0,3}\s*$String/ &&
> > - $1 !~ /_cont$/ && $1 =~ /^(?:pr|dev|netdev|netif|wiphy)_/) {
> > + $stat =~ /^\+\s*(?:if\s*$balanced_parens\s*)?($logFunctions)\s*\((?:\s*$FuncArg\s*,\s*){0,3}\s*$String/ &&
> > + $2 !~ /_cont$/ && $2 =~ /^(?:pr|dev|netdev|netif|wiphy)_/) {
> > my $cnt = statement_rawlines($stat);
> > my $extracted_string = "";
> > for (my $i = 0; $i < $cnt; $i++) {
>
> Hi Joe,
>
> This fixes the use case for drivers/usb/phy/phy-jz4770.c
>
> ./scripts/checkpatch.pl -f drivers/usb/gadget/udc/tegra-xudc.c
>
> is missing line 691.
Turns out checkpatch also considers a close brace
followed by another statement block a single statement
so that test could be:
$stat =~ /^\+\s*\}?\s*(?:if\s*$balanced_parens\s*)?($logFunctions)\s*\((?:\s*$FuncArg\s*,\s*){0,3}\s*$String/ &&
$2 !~ /_cont$/ && $2 =~ /^(?:pr|dev|netdev|netif|wiphy)_/) {
But that seems odd and I wonder if Andy agrees
so I have a question for Andy about the use of
ctx_statement_block and its implementation.
It seems that $suppress_statement should be updated
when the cond return value does not start with { so
that code like
struct foo *
function(
int arg1,
long arg2,
struct bar *baz)
{
[implementation...]
}
allows skipping the creation of a new $stat for each
line of the function arguments. Instead there would
be just 2 $stat blocks created, one for the function
with its args and implementation, another for just
the implementation with braces.
Perhaps this is proper, but perhaps the $line_nr_next
return value should be updated in ctx_statement_block
instead.
Also a $stat that starts with a close brace is odd
and probably not good.
Thoughts?
---
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 54eaa7..8ef95b 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3558,7 +3558,10 @@ sub process {
($stat, $cond, $line_nr_next, $remain_next, $off_next) =
ctx_statement_block($linenr, $realcnt, 0);
$stat =~ s/\n./\n /g;
- $cond =~ s/\n./\n /g;
+ my $condcnt = $cond =~ s/\n./\n /g;
+ if ($cond !~ /^.\s*\}/) {
+ $suppress_statement = $linenr + $condcnt;
+ }
#print "linenr<$linenr> <$stat>\n";
# If this statement has no statement boundaries within
