Re: [PATCH] checkpatch: Add a --strict test for macro argument reuse and precedence
On Sat, 2016-09-03 at 15:20 -0700, Joe Perches wrote: > Add a test for reuse of macro arguments to highlight any possible > side-effects from this reuse. > > Avoid this check on token name pasting and when the > argument is used in a typeof or a __builtin. > > Add a test for macro arguents that have leading or trailing operators > where the argument isn't parenthesized to avoid possible precedence > issues. > > These tests are noisy so make them --strict. This should have been RFC. Please do not apply this. These tests are not just noisy, some false positives it reports are just silly.
Re: [PATCH] checkpatch: Add a --strict test for macro argument reuse and precedence
On Sat, 2016-09-03 at 15:20 -0700, Joe Perches wrote: > Add a test for reuse of macro arguments to highlight any possible > side-effects from this reuse. > > Avoid this check on token name pasting and when the > argument is used in a typeof or a __builtin. > > Add a test for macro arguents that have leading or trailing operators > where the argument isn't parenthesized to avoid possible precedence > issues. > > These tests are noisy so make them --strict. This should have been RFC. Please do not apply this. These tests are not just noisy, some false positives it reports are just silly.
[PATCH] checkpatch: Add a --strict test for macro argument reuse and precedence
Add a test for reuse of macro arguments to highlight any possible side-effects from this reuse. Avoid this check on token name pasting and when the argument is used in a typeof or a __builtin. Add a test for macro arguents that have leading or trailing operators where the argument isn't parenthesized to avoid possible precedence issues. These tests are noisy so make them --strict. Signed-off-by: Joe Perches--- This is a slight expansion of a patch I sent a few years back. https://lkml.org/lkml/2012/11/6/151 This one at least runs. Maybe it's useful to find some of the possible macro misuses, but it is _very_ noisy. scripts/checkpatch.pl | 47 +++ 1 file changed, 39 insertions(+), 8 deletions(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 8946904..921155f 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -4714,7 +4714,17 @@ sub process { $has_flow_statement = 1 if ($ctx =~ /\b(goto|return)\b/); $has_arg_concat = 1 if ($ctx =~ /\#\#/ && $ctx !~ /\#\#\s*(?:__VA_ARGS__|args)\b/); - $dstat =~ s/^.\s*\#\s*define\s+$Ident(?:\([^\)]*\))?\s*//; + $dstat =~ s/^.\s*\#\s*define\s+$Ident(\([^\)]*\))?\s*//; + my $define_args = $1; + my $define_stmt = $dstat; + my @def_args = (); + + if (defined $define_args && $define_args ne "") { + $define_args = substr($define_args, 1, length($define_args) - 2); + $define_args =~ s/\s*//g; + @def_args = split(",", $define_args); + } + $dstat =~ s/$;//g; $dstat =~ s/\\\n.//g; $dstat =~ s/^\s*//s; @@ -4750,6 +4760,15 @@ sub process { ^\[ }x; #print "REST<$rest> dstat<$dstat> ctx<$ctx>\n"; + + $ctx =~ s/\n*$//; + my $herectx = $here . "\n"; + my $stmt_cnt = statement_rawlines($ctx); + + for (my $n = 0; $n < $stmt_cnt; $n++) { + $herectx .= raw_line($linenr, $n) . "\n"; + } + if ($dstat ne '' && $dstat !~ /^(?:$Ident|-?$Constant),$/ && # 10, // foo(), $dstat !~ /^(?:$Ident|-?$Constant);$/ && # foo(); @@ -4765,13 +4784,6 @@ sub process { $dstat !~ /^\(\{/ && # ({... $ctx !~ /^.\s*#\s*define\s+TRACE_(?:SYSTEM|INCLUDE_FILE|INCLUDE_PATH)\b/) { - $ctx =~ s/\n*$//; - my $herectx = $here . "\n"; - my $cnt = statement_rawlines($ctx); - - for (my $n = 0; $n < $cnt; $n++) { - $herectx .= raw_line($linenr, $n) . "\n"; - } if ($dstat =~ /;/) { ERROR("MULTISTATEMENT_MACRO_USE_DO_WHILE", @@ -4780,6 +4792,25 @@ sub process { ERROR("COMPLEX_MACRO", "Macros with complex values should be enclosed in parentheses\n" . "$herectx"); } + + } +# check if any macro arguments are reused or may have other precedence issues + foreach my $arg (@def_args) { + next if ($arg =~ /\.\.\./); + my $tmp = $define_stmt; + $tmp =~ s/\b(typeof|__typeof__|__builtin\w+|typecheck\s*\(\s*$Type\s*,|\#+)\s*\(*\s*$arg\s*\)*\b//g; + $tmp =~ s/\#\#\s*$arg\b//g; + $tmp =~ s/\b$arg\s*\#\#//g; + my $use_cnt = $tmp =~ s/\b$arg\b//g; + if ($use_cnt > 1) { + CHK("MACRO_ARG_REUSE", + "Macro argument reuse '$arg' - possible side-effects?\n" . "$herectx"); + } + if ($define_stmt =~ m/\b$arg\b\s*$Operators/m || + $define_stmt =~ /$Operators\s*$arg\b/m) { + CHK("MACRO_ARG_PRECEDENCE", + "Macro argument '$arg' may be better as '($arg)' to avoid precedence issues\n" . "$herectx"); + }
[PATCH] checkpatch: Add a --strict test for macro argument reuse and precedence
Add a test for reuse of macro arguments to highlight any possible side-effects from this reuse. Avoid this check on token name pasting and when the argument is used in a typeof or a __builtin. Add a test for macro arguents that have leading or trailing operators where the argument isn't parenthesized to avoid possible precedence issues. These tests are noisy so make them --strict. Signed-off-by: Joe Perches --- This is a slight expansion of a patch I sent a few years back. https://lkml.org/lkml/2012/11/6/151 This one at least runs. Maybe it's useful to find some of the possible macro misuses, but it is _very_ noisy. scripts/checkpatch.pl | 47 +++ 1 file changed, 39 insertions(+), 8 deletions(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 8946904..921155f 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -4714,7 +4714,17 @@ sub process { $has_flow_statement = 1 if ($ctx =~ /\b(goto|return)\b/); $has_arg_concat = 1 if ($ctx =~ /\#\#/ && $ctx !~ /\#\#\s*(?:__VA_ARGS__|args)\b/); - $dstat =~ s/^.\s*\#\s*define\s+$Ident(?:\([^\)]*\))?\s*//; + $dstat =~ s/^.\s*\#\s*define\s+$Ident(\([^\)]*\))?\s*//; + my $define_args = $1; + my $define_stmt = $dstat; + my @def_args = (); + + if (defined $define_args && $define_args ne "") { + $define_args = substr($define_args, 1, length($define_args) - 2); + $define_args =~ s/\s*//g; + @def_args = split(",", $define_args); + } + $dstat =~ s/$;//g; $dstat =~ s/\\\n.//g; $dstat =~ s/^\s*//s; @@ -4750,6 +4760,15 @@ sub process { ^\[ }x; #print "REST<$rest> dstat<$dstat> ctx<$ctx>\n"; + + $ctx =~ s/\n*$//; + my $herectx = $here . "\n"; + my $stmt_cnt = statement_rawlines($ctx); + + for (my $n = 0; $n < $stmt_cnt; $n++) { + $herectx .= raw_line($linenr, $n) . "\n"; + } + if ($dstat ne '' && $dstat !~ /^(?:$Ident|-?$Constant),$/ && # 10, // foo(), $dstat !~ /^(?:$Ident|-?$Constant);$/ && # foo(); @@ -4765,13 +4784,6 @@ sub process { $dstat !~ /^\(\{/ && # ({... $ctx !~ /^.\s*#\s*define\s+TRACE_(?:SYSTEM|INCLUDE_FILE|INCLUDE_PATH)\b/) { - $ctx =~ s/\n*$//; - my $herectx = $here . "\n"; - my $cnt = statement_rawlines($ctx); - - for (my $n = 0; $n < $cnt; $n++) { - $herectx .= raw_line($linenr, $n) . "\n"; - } if ($dstat =~ /;/) { ERROR("MULTISTATEMENT_MACRO_USE_DO_WHILE", @@ -4780,6 +4792,25 @@ sub process { ERROR("COMPLEX_MACRO", "Macros with complex values should be enclosed in parentheses\n" . "$herectx"); } + + } +# check if any macro arguments are reused or may have other precedence issues + foreach my $arg (@def_args) { + next if ($arg =~ /\.\.\./); + my $tmp = $define_stmt; + $tmp =~ s/\b(typeof|__typeof__|__builtin\w+|typecheck\s*\(\s*$Type\s*,|\#+)\s*\(*\s*$arg\s*\)*\b//g; + $tmp =~ s/\#\#\s*$arg\b//g; + $tmp =~ s/\b$arg\s*\#\#//g; + my $use_cnt = $tmp =~ s/\b$arg\b//g; + if ($use_cnt > 1) { + CHK("MACRO_ARG_REUSE", + "Macro argument reuse '$arg' - possible side-effects?\n" . "$herectx"); + } + if ($define_stmt =~ m/\b$arg\b\s*$Operators/m || + $define_stmt =~ /$Operators\s*$arg\b/m) { + CHK("MACRO_ARG_PRECEDENCE", + "Macro argument '$arg' may be better as '($arg)' to avoid precedence issues\n" . "$herectx"); + } } # check for