diff --git a/.github/actions/checkpatch/action.yml b/.github/actions/checkpatch/action.yml index a270700..db837be 100644 --- a/.github/actions/checkpatch/action.yml +++ b/.github/actions/checkpatch/action.yml @@ -7,9 +7,11 @@ inputs: runs: using: composite steps: - - uses: actions/checkout@v2 + - uses: actions/checkout@v3 with: repository: tarantool/checkpatch path: 'checkpatch' - - run: checkpatch/checkpatch.pl --color=always --show-types --git ${{ inputs.revision-range }} + - run: apt install -y codespell + shell: bash + - run: checkpatch/checkpatch.pl --codespell --color=always --show-types --git ${{ inputs.revision-range }} shell: bash diff --git a/checkpatch.pl b/checkpatch.pl index 67d8d25..e2049ac 100755 --- a/checkpatch.pl +++ b/checkpatch.pl @@ -15,6 +15,7 @@ use Cwd 'abs_path'; use Term::ANSIColor qw(:constants); use Encode qw(decode encode); +use File::Spec::Functions 'catfile'; my $P = $0; my $D = dirname(abs_path($P)); @@ -370,17 +371,21 @@ sub hash_show_words { my $emitted_corrupt = 0; -our $Ident = qr{ - (?:::)?(?:[A-Za-z_][A-Za-z\d_]*::)* +our $IdentNoNs = qr{ [A-Za-z_][A-Za-z\d_]* (?:\s*\#\#\s*[A-Za-z_][A-Za-z\d_]*)* }x; +our $Ident = qr{ + (?:::)?(?:[A-Za-z_][A-Za-z\d_]*::)* + $IdentNoNs + }x; our $Storage = qr{extern|static}; our $Attribute = qr{ typename| const| volatile| alignas| + ALWAYS_INLINE| API_EXPORT| CFORMAT| DEPREACTED| @@ -388,6 +393,7 @@ sub hash_show_words { MAYBE_UNUSED| NODISCARD| NORETURN| + NO_SANITIZE_ADDRESS| PACKED }x; our $Modifier; @@ -442,13 +448,12 @@ sub hash_show_words { # Skip all checks for the following paths. our $skipPaths = qr{(?x: + src\/lib\/tzcode\/| + third_party\/| test\/static\/corpus\/ )}; -# Skip C/C++ source code checks for the following paths. -our $skipSrcPaths = qr{(?x: - src\/lib\/tzcode\/ -)}; +our $generatedFiles = ''; our $typeMacros = qr{(?x: LIGHT @@ -495,6 +500,36 @@ sub hash_show_words { NO_CHANGELOG )}; +our $github_ref = qr{(https?:\/\/)?(www\.)?github\.com\b([-a-zA-Z0-9()@:%_\+.~\#?&\/=]*)}; +our $github_issue_ref = qr{((tarantool\/\S+)?\#[1-9][0-9]*)}; + +our $github_issue_markers = qr{(?x: + Close| + Closes| + Part\s+of| + Part-of| + Issue| + Follow\s+up| + Follows\s+up| + Follow-up| + Follows-up| + Needed\s+for| + Needed-for| + Fix| + Fixes| + Fixed| + See| + See\s+also| + See-also| + Related\s+to| + Related-to| + Related| + Relates| + Workaround| + Implement| + Implements +)}; + sub edit_distance_min { my (@arr) = @_; my $len = scalar @arr; @@ -573,6 +608,7 @@ sub find_standard_signature { ); our @typeList = ( + qr{auto}, qr{void}, qr{(?:(?:un)?signed\s+)?char}, qr{(?:(?:un)?signed\s+)?short\s+int}, @@ -593,6 +629,7 @@ sub find_standard_signature { qr{${Ident}_f}, qr{${Ident}_fn}, qr{${Ident}_cb}, + qr{(?:::)?(?:[A-Z][a-z][A-Za-z\d]*::)*[A-Z][a-z][A-Za-z\d]*(?!::)}, @typeListMisordered, ); @@ -884,6 +921,32 @@ sub git_commit_info { @ARGV = @commits; } +# Collect files marked as generated in .gitattributes +if ($git && $gitroot) { + # $gitroot points to a .git directory/file inside a repository + # root. Calculate its parent directory -- the repository root. + # + # TODO: Should we handle a bare git repository somehow? + my $repo_root = dirname(abs_path($gitroot)); + + if (open(my $gitattributes, '<', catfile($repo_root, ".gitattributes"))) { + my @acc = (); + while (<$gitattributes>) { + my $line = $_; + # https://docs.github.com/en/repositories/working-with-files/managing-files/customizing-how-changed-files-appear-on-github + next if ($line !~ /linguist-generated=true/); + $line =~ s/ +[^ ]+$//; + # https://www.git-scm.com/docs/gitignore#_pattern_format + # + # Assume that the file pattern is just a file path or + # a directory path in the repository. + push(@acc, "^\Q$line\E"); + } + close($gitattributes); + $generatedFiles = join("|", @acc); + } +} + my $vname; for my $filename (@ARGV) { my $FILE; @@ -1582,7 +1645,9 @@ sub get_stat_real { my $stat_real = raw_line($linenr, 0); for (my $count = $linenr + 1; $count <= $lc; $count++) { - $stat_real = $stat_real . "\n" . raw_line($count, 0); + my $rl = raw_line($count, 0); + last if !defined($rl); + $stat_real = $stat_real . "\n" . $rl; } return $stat_real; @@ -1593,7 +1658,9 @@ sub get_stat_here { my $herectx = $here . "\n"; for (my $n = 0; $n < $cnt; $n++) { - $herectx .= raw_line($linenr, $n) . "\n"; + my $rl = raw_line($linenr, $n); + last if !defined($rl); + $herectx .= $rl . "\n"; } return $herectx; @@ -2048,6 +2115,7 @@ sub process { my $commit_log_lines = 0; #Number of commit log lines my $commit_log_long_line = 0; my $commit_log_no_wrap = 0; + my $commit_log_code_section = 0; my $commit_log_has_diff = 0; my $non_utf8_charset = 0; my $has_exec_perm = 0; #Current file has exec permissions @@ -2069,6 +2137,7 @@ sub process { my $here = ''; my $context_function; #undef'd unless there's a known function my $context_struct; #undef'd unless there's a known struct + my $in_template_indent; #nddef'd unless in template arguments my $in_comment = 0; my $first_line = 0; my %check_comment_ignore = (); @@ -2205,6 +2274,7 @@ sub process { } else { undef $context_struct; } + undef $in_template_indent; next; # track the line number as we move through the hunk, note that @@ -2261,7 +2331,7 @@ sub process { if ($line =~ /^new (file )?mode.*[7531]\d{0,2}$/) { $has_exec_perm = 1 } - if ($realline == 1 and ($line =~ /^\+#!/) != $has_exec_perm) { + if ($realline == 1 and ($rawline =~ /^\+#!/) != $has_exec_perm) { my $permhere = $here . "FILE: $realfile\n"; my $msg = ($has_exec_perm ? "Executable file must have hashbang (#!)" : @@ -2483,13 +2553,6 @@ sub process { } } -# Check email subject for common tools that don't need to be mentioned - if ($in_header_lines && - $line =~ /^Subject:.*\b(?:checkpatch|sparse|smatch)\b[^:]/i) { - ERROR("EMAIL_SUBJECT", - "A patch subject line should describe the change not the tool that found it\n" . $herecurr); - } - # Check for Gerrit Change-Ids not in any patch context if ($realfile eq '' && !$has_patch_separator && $line =~ /^\s*change-id:/i) { ERROR("GERRIT_CHANGE_ID", @@ -2499,9 +2562,12 @@ sub process { if ($in_commit_log && $line =~ /^NO_WRAP$/) { $commit_log_no_wrap = !$commit_log_no_wrap; } + if ($in_commit_log && $line =~ /^```/) { + $commit_log_code_section = !$commit_log_code_section; + } # Check for line lengths > 75 in commit log, warn once - if ($in_commit_log && !$commit_log_no_wrap && !$commit_log_long_line && + if ($in_commit_log && !$commit_log_no_wrap && !$commit_log_code_section && !$commit_log_long_line && length($line) > 75 && !($line =~ /^\s*[a-zA-Z0-9_\/\.]+\s+\|\s+\d+/ || # file delta changes @@ -2548,7 +2614,7 @@ sub process { my $input = $line; if ($line =~ /(?:\bcommit\s+[0-9a-f]{5,}|\bcommit\s*$)/i) { - for (my $n = 0; $n < 2; $n++) { + for (my $n = 0; $n < 3; $n++) { if ($input =~ /\bcommit\s+[0-9a-f]{5,}\s*($balanced_parens)/i) { $orig_desc = $1; $has_parens = 1; @@ -2598,8 +2664,6 @@ sub process { $herecurr) if (!$emitted_corrupt++); } - next if !$file and $realfile =~ /^$skipPaths/; - # UTF-8 regex found at http://www.w3.org/International/questions/qa-forms-utf-8.en.php if (($realfile =~ /^$/ || $line =~ /^\+/) && $rawline !~ m/^$UTF8*$/) { @@ -2637,11 +2701,21 @@ sub process { "8-bit UTF-8 used in possible commit log\n" . $herecurr); } + next if $realfile ne '' && $realfile =~ /^$skipPaths/; + +# Skip files marked as generated in .gitattributes + next if $realfile ne '' && $generatedFiles ne '' && $realfile =~ $generatedFiles; + # Check for various typo / spelling mistakes if (defined($misspellings) && ($in_commit_log || $line =~ /^(?:\+|Subject:)/i)) { - while ($rawline =~ /(?:^|[^\w\-'`])($misspellings)(?:[^\w\-'`]|$)/gi) { - my $typo = $1; + while ($rawline =~ /(^|[^\w\-'`])($misspellings)(?:[^\w\-'`]|$)/gi) { + my $prev = $1; + my $typo = $2; + # Ignore if this is a patch file and the line is removed or unchanged. + next if $realfile =~ /\.patch$/ && $rawline !~ /^\+\+/; + # Ignore anything that looks like regular expression or file path. + next if $prev =~ /[.|\/]/; # Ignore ALL UPPER CASE words as it may be an abbreviation, # e.g. TAHT - Tahiti Time next if ($typo eq uc($typo)); @@ -2650,6 +2724,10 @@ sub process { my $hereptr = "$hereline$ptr\n"; my $typo_fix = $spelling_fix{lc($typo)}; $typo_fix = ucfirst($typo_fix) if ($typo =~ /^[A-Z]/); + # Ignore CamelCase. + my $typo_fix_camel_case = $typo_fix; + $typo_fix_camel_case =~ s/ (\w)/\U$1/g; + next if ($typo eq $typo_fix_camel_case); $typo_fix = uc($typo_fix) if ($typo =~ /^[A-Z]+$/); # Ignore the case when we drop a character to use a misspelled word as function name, e.g. isnt(). next if ($realfile =~ /\.h|c|cc|lua$/ && $typo_fix =~ /[^a-zA-Z_]/ && $line =~ /\b$typo\s*\(/); @@ -2671,6 +2749,14 @@ sub process { if ($in_commit_log && $line =~ /^($custom_tags)=/) { $commit_log_tags{$1} = 1; + if ($has_doc) { + ERROR("TAG_IN_DOC", + "Please move $1 tag before doc request\n"); + } + } + if ($in_commit_log && $has_doc && $line =~ /^($github_issue_markers)\s*:?\s*($github_issue_ref|$github_ref)/) { + ERROR("ISSUE_IN_DOC", + "Please move \"$line\" issue reference before doc request\n"); } if ($in_commit_log && $line =~ /^\@TarantoolBot document$/) { $has_doc = 1; @@ -2682,9 +2768,11 @@ sub process { $has_test = 1; } + $is_test = ($realfile =~ /^(?:test|perf)\//); + # check for repeated words separated by a single space # avoid false positive from list command eg, '-rw-r--r-- 1 root root' - if (($rawline =~ /^\+/ || $in_commit_log) && + if (($rawline =~ /^\+/ || ($in_commit_log && !$commit_log_code_section)) && $rawline !~ /[bcCdDlMnpPs\?-][rwxsStT-]{9}/) { pos($rawline) = 1 if (!$in_commit_log); while ($rawline =~ /\b($word_pattern) (?=($word_pattern))/g) { @@ -2693,7 +2781,7 @@ sub process { my $second = $2; my $start_pos = $-[1]; my $end_pos = $+[2]; - if ($first =~ /(?:struct|union|enum)/) { + if ($first =~ /(?:class|struct|union|enum)/) { pos($rawline) += length($first) + length($second) + 1; next; } @@ -2705,6 +2793,13 @@ sub process { # @param request Request to process next if $rawline =~ /^\+\s\*\s+[\@\\]param(?:\[[a-z,]*\])*\s+$first $second/; + # Ignore variable declarations. + next if $realfile =~ /.h|c|cc|proto$/ && + $rawline =~ /^\+\s*(?:(?:$Storage|$Modifier|optional|required|repeated)\s+)*$first\s+$second\s*[=;]/; + + # Ignore 'not not' in Lua. + next if $realfile =~ /\.lua$/ && $first eq 'not'; + # check for character before and after the word matches my $start_char = ''; my $end_char = ''; @@ -2759,8 +2854,18 @@ sub process { "adding a line without newline at end of file\n" . $herecurr); } +# Ban non-ASCII characters in text files other than tests + if (!$is_test && $rawline =~ /^\+.*$NON_ASCII_UTF8/) { + my $l = $rawline; + $l =~ s/$NON_ASCII_UTF8.*//; + my $ptr = copy_spacing($l) . "^"; + my $hereptr = "$hereline$ptr\n"; + ERROR("NON_ASCII_CHAR", + "please, don't use non-ASCII characters\n" . $hereptr); + } + # check for space before tabs. - if ($rawline =~ /^\+/ && $rawline =~ / \t/) { + if ($realfile !~ /\.patch$/ && $rawline =~ /^\+/ && $rawline =~ / \t/) { my $herevet = "$here\n" . cat_vet($rawline) . "\n"; ERROR("SPACE_BEFORE_TAB", "please, no space before tabs\n" . $herevet); @@ -2781,19 +2886,6 @@ sub process { "Please avoid new tests with .result files\n"); } -# check we are in a valid C source file if not then ignore this hunk - next if ($realfile !~ /\.(h|c|cc)$/); - -# ignore C source files outside the source and test directories when checking patches - next if !$file and ($realfile !~ /^(?:src|test|perf)\// || $realfile =~ /^$skipSrcPaths/); - - $is_test = ($realfile =~ /^(?:test|perf)\//); - - if ($line =~ /^\+\s*#\s*ifndef\s+[A-Z0-9_]+(?:_H|_INCLUDED)\s*$/) { - ERROR("INCLUDE_GUARD", - "Please use '#pragma once' instead of include guard macro\n" . $herecurr); - } - # line length limit (with some exclusions) # # There are a few types of lines that may extend beyond $max_line_length: @@ -2812,7 +2904,8 @@ sub process { # if LONG_LINE is ignored, the other 2 types are also ignored # - if ($line =~ /^\+/ && $length > $max_line_length) { + if ($realfile =~ /\.(h|c|cc|lua)$/ && $realfile !~ /\.test\.lua$/ && + $line =~ /^\+/ && $length > $max_line_length) { my $msg_type = "LONG_LINE"; # Check the allowed long line types first @@ -2844,8 +2937,8 @@ sub process { } elsif ($line =~ /^\+.*\\$/ && $length == $max_line_length + 1) { $msg_type = ""; # Comment which is used as a header in array initializer - # /* FOO BAR BAZ */ - } elsif ($rawline =~ /^.\s*\/\*[a-zA-Z_\s]+\*\/\s*$/) { + # /* FOO BAR 12 34 */ + } elsif ($rawline =~ /^.\s*\/\*[0-9a-zA-Z_\s]+\*\/\s*$/) { $msg_type = ""; # Otherwise set the alternate message types @@ -2867,6 +2960,15 @@ sub process { } } +# check we are in a valid C source file if not then ignore this hunk + next if ($realfile !~ /\.(h|c|cc)$/); + +# require pragma instead of include guards + if ($line =~ /^\+\s*#\s*ifndef\s+[A-Z0-9_]+(?:_H|_INCLUDED)\s*$/) { + ERROR("INCLUDE_GUARD", + "Please use '#pragma once' instead of include guard macro\n" . $herecurr); + } + # at the beginning of a line any tabs must come first and anything # more than $tabsize must use tabs. if ($rawline =~ /^\+\s* \t\s*\S/ || @@ -2884,6 +2986,31 @@ sub process { "code indent shouldn't use spaces if the previous line ends with $what\n" . $herevet); } +# alignment should match the comment on the previous line unless it's the end of block +# or a special comment that starts with '/****' used to separate logical code parts + if ($prevrawline !~ /^.\/\*\*\*\*/ && + $prevrawline =~ /^.(\s*)(?:\/\*.*| )\*\/\s*$/) { + my $ident = length($1); + if ($rawline =~ /^\+(\s*)[^}\s]/ && length($1) != $ident) { + ERROR("CODE_IDENT", + "code indent should match comment\n" . $hereprev); + } + } + +# check function argument alignment after opening parenthesis + if ($prevrawline =~ /^.(\s*)((?:if|while|for|switch)\s*\(.*)?\b$Ident\s*\(\s*$/) { + my $previndent = length(expand_tabs($1)); + my $in_cond = defined($2); + my $expected_tabs = ceil($previndent / $tabsize) + ($in_cond ? 2 : 1); + if ($rawline =~ /^\+(\s*)/) { + my $indent = length(expand_tabs($1)); + if ($indent != $expected_tabs * $tabsize) { + ERROR("CODE_INDENT", + "bad code indent, should be $expected_tabs tabs\n" . $hereprev); + } + } + } + # check for assignments on the start of a line if ($sline =~ /^\+\s+($Assignment)[^=]/) { ERROR("ASSIGNMENT_CONTINUATIONS", @@ -2897,7 +3024,7 @@ sub process { } # check indentation starts on a tab stop - if ($sline =~ /^\+\t+( +)(?:$c90_Keywords\b|\{\s*$|\}\s*(?:else\b|while\b|\s*$)|$Declare\s*$Ident\s*[;=])/) { + if (!defined($in_template_indent) && $sline =~ /^\+\t+( +)(?:$c90_Keywords\b|\{\s*$|\}\s*(?:else\b|while\b|\s*$)|$Declare\s*$Ident\s*[;=])/) { my $indent = length($1); if ($indent % $tabsize) { ERROR("TABSTOP", @@ -2905,8 +3032,30 @@ sub process { } } +# check template arguments alignment + if (defined($in_template_indent)) { + if ($rawline =~ /^\+(\s*)/) { + my $indent = length(expand_tabs($1)); + if ($indent != $in_template_indent) { + ERROR("CODE_INDENT", + "unaligned template arguments\n" . $hereprev); + } + } + if ($rawline =~ />\s*$/) { + undef $in_template_indent; + } + } elsif ($rawline =~ /^\+(\s*)template(\s*) 0) { + ERROR("SPACING", + "space prohibited between 'template' and '<'\n" . $herecurr); + } + if ($rawline !~ />\s*\\?$/) { + $in_template_indent = length(expand_tabs($1)) + length('template<'); + } + } + # check multi-line statement indentation matches previous line - if ($prevline =~ /^\+([ \t]*)((?:$c90_Keywords(?:\s+if)\s*)|(?:$Declare\s*)?(?:$Ident|\(\s*\*\s*$Ident\s*\))\s*|(?:(?:\*\s*)*$Lval|$Declare\s*$Ident)\s*=\s*$Ident\s*)\(.*(\&\&|\|\||,)\s*$/) { + if ($prevline =~ /^\+([ \t]*)((?:$c90_Keywords(?:\s+if)\s*)|(?:$Declare\s*|return\s*)?(?:$Ident|\(\s*\*\s*$Ident\s*\))\s*|(?:(?:\*\s*)*$Lval|$Declare\s*$Ident)\s*=\s*$Ident\s*)\(.*($Operators|"|\))\s*$/) { $prevline =~ /^\+(\t*)(.*)$/; my $oldindent = $1; my $rest = $2; @@ -3001,13 +3150,13 @@ sub process { # check for space after cast like "(int) foo" or "(struct foo) bar" # avoid checking a few false positives: -# "sizeof()" or "__alignof__()" +# "()" # function pointer declarations like "(*foo)(int) = bar;" # structure definitions like "(struct foo) { 0 };" # multiline macros that define functions # known attributes or the __attribute__ keyword if ($line =~ /^\+(.*)\(\s*$Type\s*\)([ \t]++)((?![={]|\\$|$Attribute|__attribute__))/ && - (!defined($1) || $1 !~ /\b(?:sizeof|__alignof__)\s*$/)) { + (!defined($1) || $1 !~ /\b$Ident\s*$/)) { ERROR("SPACING", "No space is necessary after a cast\n" . $herecurr); } @@ -3452,7 +3601,7 @@ sub process { } # check for initialisation to aggregates open brace on the next line - if ($line =~ /^.\s*{/ && + if ($line =~ /^.\s*{/ && $line !~ /}\s*;\s*$/ && $prevline =~ /(?:^|[^=])=\s*$/) { ERROR("OPEN_BRACE", "that open brace { should be on the previous line\n" . $hereprev); @@ -3517,8 +3666,8 @@ sub process { "const array should probably be static const\n" . $herecurr); } -# check for static char foo[] = "bar" declarations. - if ($line =~ /\bstatic\s+char\s+(\w+)\s*\[\s*\]\s*=\s*"/) { +# check for static char foo[] = "bar" declarations; ignore mkdtemp template + if ($line =~ /\bstatic\s+char\s+(\w+)\s*\[\s*\]\s*=\s*"/ && $line !~ /XXXXXX/) { ERROR("STATIC_CONST_CHAR_ARRAY", "static char array declaration should probably be static const char\n" . $herecurr); @@ -3569,8 +3718,8 @@ sub process { } # check that function name and return value type are placed on different lines - if ($realfile =~ /\bbox\.h$/ && $line =~ /^\+\s*$Declare\s*box_set_/) { - # ignore box_set_XXX in box.h + if ($line =~ /^\+\s*$Declare\s*(?:box_set_|generic_)/) { + # ignore box_set_XXX and generic_XXX } elsif ($line =~ /^\+(?:typedef\s+)?$Declare\s*(?:$Ident|\(\s*\*\s*$Ident\s*\))\s*\(/) { ERROR("FUNCTION_NAME_NO_NEWLINE", "Function name and return value type should be placed on different lines\n" . $herecurr) @@ -3840,7 +3989,7 @@ sub process { ERROR("SPACING", "space prohibited before that '$op' $at\n" . $hereptr); } - if ($ctx !~ /.x[WEC]/ && $cc !~ /^}/) { + if ($ctx !~ /.x[WECB]/ && $cc !~ /^}/) { ERROR("SPACING", "space required after that '$op' $at\n" . $hereptr); } @@ -4074,6 +4223,12 @@ sub process { } } +# check for a closing parenthesis on a separate line + if ($line =~ /^\+\s*\)\s*[;{]?\s*$/) { + ERROR("DANGLING_PARENTHESIS", + "Closing parenthesis should follow function argument or operand\n" . $herecurr); + } + # comparisons with a constant or upper case identifier on the left # avoid cases like "foo + BAR < baz" # only fix matches surrounded by parentheses to avoid incorrect @@ -4082,7 +4237,7 @@ sub process { my $lead = $1; my $to = $2; if ($lead !~ /(?:$Operators|\.)\s*$/ && - $to !~ /^(?:Constant|[A-Z_][A-Z0-9_]*)$/) { + $to !~ /^(?:$Constant|[A-Z_][A-Z0-9_]*)\s*$/) { ERROR("CONSTANT_COMPARISON", "Comparisons should place the constant on the right side of the test\n" . $herecurr); } @@ -4283,7 +4438,7 @@ sub process { my $stmt_cnt = statement_rawlines($ctx); my $herectx = get_stat_here($linenr, $stmt_cnt, $here); - if ($dstat ne '' && + if (!$is_test && $dstat ne '' && !($dstat =~ /,/ && $dstat !~ /;/) && # comma-separated list used as array initializer $dstat !~ /^(?:$Ident|-?$Constant),$/ && # 10, // foo(), $dstat !~ /^(?:$Ident|-?$Constant);$/ && # foo(); @@ -4292,6 +4447,7 @@ sub process { $dstat !~ /$exceptions/ && $dstat !~ /^\.$Ident\s*=/ && # .foo = $dstat !~ /^(?:\#\s*$Ident|\#\s*$Constant)\s*$/ && # stringification #foo + $dstat !~ /^(?:$Ident\s*)+$/ && # compound code generation macros: foo() bar() buzz() $dstat !~ /^do\s*$Constant\s*while\s*$Constant;?$/ && # do {...} while (...); // do {...} while (...) $dstat !~ /^while\s*$Constant\s*$Constant\s*$/ && # while (...) {...} $dstat !~ /^for\s*$Constant$/ && # for (...) @@ -4331,16 +4487,19 @@ sub process { # check if any macro arguments are reused (ignore '...' and 'type') foreach my $arg (@def_args) { - next if ($arg =~ /\.\.\./); - next if ($arg =~ /^type$/i); + next if ($arg =~ /\.\.\./); # Skip the ellipsis. + next if ($arg =~ /^type$/i); # Skip the argument with a special name. + next if ($define_stmt =~ /\b$arg\s*::/); # Skip the argument if it's used as a scope name (it's a class or a namespace name). my $tmp_stmt = $define_stmt; $tmp_stmt =~ s/\b(__must_be_array|offsetof|sizeof|sizeof_field|__stringify|typeof|__typeof__|__builtin\w+|typecheck\s*\(\s*$Type\s*,|\#+)\s*\(*\s*$arg\s*\)*\b//g; $tmp_stmt =~ s/\#+\s*$arg\b//g; $tmp_stmt =~ s/\b$arg\s*\#\#//g; # check if any macro arguments may have other precedence issues - if ($tmp_stmt =~ m/($Operators)?\s*\b$arg\b\s*($Operators)?/m && - ((defined($1) && $1 ne ',') || - (defined($2) && $2 ne ','))) { + if ($tmp_stmt =~ m/(\()?\s*($Operators)?\s*\b$arg\b\s*($Operators)?[\s\*]*(\))?/m && + ((defined($2) && $2 ne ',') || + (defined($3) && $3 ne ',')) && + !(defined($2) && ($2 eq '->' || $2 eq '.')) && + !(defined($1) && !defined($2) && (!defined($3) || $3 eq '*') && defined($4))) { ERROR("MACRO_ARG_PRECEDENCE", "Macro argument '$arg' may be better as '($arg)' to avoid precedence issues\n" . "$herectx"); } @@ -4348,7 +4507,7 @@ sub process { # check for macros with flow control, but without ## concatenation # ## concatenation is commonly a macro that defines a function so ignore those - if ($has_flow_statement && !$has_arg_concat) { + if (!$is_test && $has_flow_statement && !$has_arg_concat) { my $cnt = statement_rawlines($ctx); my $herectx = get_stat_here($linenr, $cnt, $here); @@ -4360,6 +4519,7 @@ sub process { } else { if ($prevline !~ /^..*\\$/ && + $rawline !~ /^\+\s*\*\/\s*\\$/ && # multiline comment $line !~ /^\+\s*\#.*\\$/ && # preprocessor $line !~ /^\+.*\b(__asm__|asm)\b.*\\$/ && # asm $line =~ /^\+.*\\$/) { @@ -4490,7 +4650,7 @@ sub process { # no volatiles please my $asm_volatile = qr{\b(__asm__|asm)\s+(__volatile__|volatile)\b}; - if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/ && $line !~ /\bsig_atomic_t\b/) { + if (!$is_test && $line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/ && $line !~ /\bsig_atomic_t\b/) { ERROR("VOLATILE", "Use of volatile is usually wrong\n" . $herecurr); } @@ -4627,7 +4787,9 @@ sub process { # operator() is used as a callback so a comment isn't necessary } elsif ($line =~ /^\+\s*(?:$Declare)?\s*(?:generic_|disabled_|exhausted_)/) { # generic_XXX, disabled_XXX, exhausted_XXX are functions are stubs so comments are not required - } elsif ($line =~ /^\+\s*($Declare)?\s*(?:($Ident)\s*\(|\(\s*\*\s*($Ident)\s*\)\s*\(|($Ident)\s*;)/) { + } elsif ($prevline =~ /[,(]\s*$/) { + # function argument list + } elsif ($line =~ /^\+\s*($Declare)?\s*(?:($Ident)\s*\(|\(\s*\*\s*($Ident)\s*\)\s*\(|($Ident)\s*;)/ && $line !~ /\\\s*$/) { # function, function pointer, variable / struct member my $decl = $1; my $is_func = defined($2); @@ -4649,6 +4811,7 @@ sub process { my $cnt = statement_rawlines($stat); for (my $n = 0; $n < $cnt; $n++) { my $rl = raw_line($linenr, $n); + last if !defined($rl); if (!defined($func_body_size)) { $func_body_size = -1 if $rl =~ /\{/; next; @@ -4660,6 +4823,8 @@ sub process { } if (!defined($decl)) { # not a declaration + } elsif (!$is_func && $decl =~ /\bstruct\s+trigger\b/) { + # ignore trigger declarations because we usually add a comment to a trigger function instead } elsif (!$is_func && defined($context_function)) { # ignore local variables } elsif ($realfile !~ /\.h$/ && $decl !~ /\bstatic\b/ && !defined($context_struct)) { @@ -4729,12 +4894,14 @@ sub process { my %attr_list = ( "aligned" => "alignas", + "always_inline" => "ALWAYS_INLINE", "format" => "CFORMAT", "deprecated" => "DEPREACTED", "fallthrough" => "FALLTHROUGH", "nodiscard" => "NODISCARD", "noinline" => "NOINLINE", "noreturn" => "NORETURN", + "no_sanitize_address" => "NO_SANITIZE_ADDRESS", "packed" => "PACKED", "unused" => "MAYBE_UNUSED" ); @@ -4845,7 +5012,7 @@ sub process { # check for function declarations that have arguments without identifier names # suppress the check for expressions in function context to avoid FP error for class object definition if (defined $stat && !defined $context_function && - $stat =~ /^.\s*(?:extern\s+)?$Type\s*(?:$Ident|\(\s*\*\s*$Ident\s*\))\s*\(\s*([^{]+)\s*\)\s*;/s && + $stat =~ /^.\s*(?:extern\s+)?$Type\s*(?:$IdentNoNs|\(\s*\*\s*$IdentNoNs\s*\))\s*\(\s*([^{]+)\s*\)\s*;/s && $1 ne "void") { my $args = trim($1); while ($args =~ m/\s*($Type\s*(?:$Ident|\(\s*\*\s*$Ident?\s*\)\s*$balanced_parens)?)/g) { @@ -4868,6 +5035,7 @@ sub process { my $herectx = $here . "\n"; for (my $n = 0; $n < $cnt; $n++) { my $rl = raw_line($linenr, $n); + last if !defined($rl); $herectx .= $rl . "\n"; $ok = 1 if ($rl =~ /^[ \+]\s*\{/); # allow opening and closing braces on the same line @@ -4914,9 +5082,10 @@ sub process { } # warn about unsafe functions - if ($line =~ /\b($Ident)\s*\(/) { + if (!$is_test && $line =~ /\b($Ident)\s*\(/) { my $func = $1; my %func_list = ( + "getenv" => "getenv_safe", "sprintf" => "snprintf", "vsprintf" => "vsnprintf", "strcpy" => "strlcpy", @@ -5104,6 +5273,10 @@ sub process { ERROR("UNTERMINATED_TAG", "Unterminated NO_WRAP section in commit description\n") } + if ($commit_log_code_section) { + ERROR("UNTERMINTATED_CODE_SECTION", + "Untermintated code section (```) in commit description\n"); + } if (!$has_doc && !exists($commit_log_tags{'NO_DOC'})) { ERROR("NO_DOC", "Please add doc or NO_DOC= tag\n"); diff --git a/doc/checkpatch.rst b/doc/checkpatch.rst index f99ab86..b44a759 100644 --- a/doc/checkpatch.rst +++ b/doc/checkpatch.rst @@ -94,7 +94,7 @@ Available options: Example:: - ./scripts/checkpatch.pl mypatch.patch --types EMAIL_SUBJECT,BRACES + ./scripts/checkpatch.pl mypatch.patch --types LINE_SPACING,BRACES - --ignore TYPE(,TYPE2...) @@ -102,7 +102,7 @@ Available options: Example:: - ./scripts/checkpatch.pl mypatch.patch --ignore EMAIL_SUBJECT,BRACES + ./scripts/checkpatch.pl mypatch.patch --ignore LINE_SPACING,BRACES - --show-types @@ -246,11 +246,6 @@ Commit message The patch is missing a commit description. A brief description of the changes made by the patch should be added. - **EMAIL_SUBJECT** - Naming the tool that found the issue is not very useful in the - subject line. A good subject line summarizes the change that - the patch brings. - **FROM_SIGN_OFF_MISMATCH** The author's email does not match with that in the Signed-off-by: line(s). This can be sometimes caused due to an improperly configured @@ -647,6 +642,10 @@ Spacing and Brackets printk(KERN_INFO "bar"); + **DANGLING_PARENTHESIS** + A closing parenthesis should never be placed on a separate line. It should + always follow a function argument or an operand. + **ELSE_AFTER_BRACE** `else {` should follow the closing block `}` on the same line. @@ -741,6 +740,11 @@ Others The memset use appears to be incorrect. This may be caused due to badly ordered parameters. Please recheck the usage. + **NON_ASCII_CHAR** + Non-ASCII characters should be avoided because they may complicate + grepping. For example, Russian 'ะก' looks exactly like English 'C' + but grep doesn't find it. + **NO_CHANGELOG** The patch lacks a changelog. Please add a new changelog entry to the changelog/unreleased directory. If the patch doesn't need a changelog diff --git a/spelling.txt b/spelling.txt index 0c8b79c..05715c6 100644 --- a/spelling.txt +++ b/spelling.txt @@ -2,6 +2,8 @@ # removed, and various additions have been made as they've been discovered # in the kernel source. # +# Tarantool authors added a few common mistakes in this checkpatch fork. +# # License: GPLv2 # # The format of each line is: @@ -51,7 +53,6 @@ acient||ancient acitions||actions acitve||active acknowldegement||acknowledgment -acknowledgement||acknowledgment ackowledge||acknowledge ackowledged||acknowledged acording||according @@ -130,6 +131,7 @@ an one||a one analysator||analyzer ang||and anniversery||anniversary +annotatoin||annotation annoucement||announcement anomolies||anomalies anomoly||anomaly @@ -1619,8 +1621,10 @@ withing||within wnat||want wont||won't workarould||workaround +writeable||writable writeing||writing writting||writing wtih||with +yeild||yield zombe||zombie zomebie||zombie