From 497c692fa2c31db4909e1d79dd95becefb5a98db Mon Sep 17 00:00:00 2001 From: Peter Schrammel Date: Thu, 8 Dec 2016 11:44:49 +0000 Subject: [PATCH 01/30] No spaces around assignment operator --- scripts/cpplint.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/cpplint.py b/scripts/cpplint.py index cade10ee3cd..0389a0e1ef8 100644 --- a/scripts/cpplint.py +++ b/scripts/cpplint.py @@ -3261,8 +3261,8 @@ def CheckOperatorSpacing(filename, clean_lines, linenum, error): # Otherwise not. Note we only check for non-spaces on *both* sides; # sometimes people put non-spaces on one side when aligning ='s among # many lines (not that this is behavior that I approve of...) - match1 = Search(r'[\s](>=|<=|==|!=|&=|\^=|\|=|\+=|\*=|\/=|\%=|>|<|\^|\+|\/)', line) - match2 = Search(r'(>=|<=|==|!=|&=|\^=|\|=|\+=|\*=|\/=|\%=|>|<|!|\^|\+|\/)[\s]', line) + match1 = Search(r'[\s](=|>=|<=|==|!=|&=|\^=|\|=|\+=|\*=|\/=|\%=|>|<|\^|\+|\/)', line) + match2 = Search(r'(=|>=|<=|==|!=|&=|\^=|\|=|\+=|\*=|\/=|\%=|>|<|!|\^|\+|\/)[\s]', line) if match1 and not Search(r'<<', line) and not Search(r'char \*', line) and not Search(r'\#include', line) and not Search(r'<.*[^\s]>', line): # and not Search(r'\b(if|while|for) ', line) # # Operators taken from [lex.operators] in C++11 standard. From 3fb7b1f47e062bee34c4340b8b42ff34a426ce51 Mon Sep 17 00:00:00 2001 From: Peter Schrammel Date: Thu, 8 Dec 2016 11:32:31 +0000 Subject: [PATCH 02/30] Improved wording --- CODING_STANDARD | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/CODING_STANDARD b/CODING_STANDARD index bd489994058..07d061c3130 100644 --- a/CODING_STANDARD +++ b/CODING_STANDARD @@ -5,8 +5,12 @@ Whitespaces: - No lines wider than 80 chars. - If a method is bigger than 50 lines, break it into parts. - Put matching { } into the same column. -- No spaces around operators, except &&, ||, and << -- Spaces after , and ; inside 'for' +- No spaces around operators (=, +, ==, ...) + Exceptions: Spaces around &&, || and << +- Space after comma (parameter lists, argument lists, ...) +- Space after colon inside 'for' +- No whitespaces at end of line +- No whitespaces in blank lines - Put argument lists on next line (and ident 2 spaces) if too long - Put parameters on separate lines (and ident 2 spaces) if too long - No whitespaces around colon for inheritance, @@ -16,8 +20,6 @@ Whitespaces: - if(...), else, for(...), do, and while(...) are always in a separate line - Break expressions in if, for, while if necessary and align them with the first character following the opening parenthesis -- No whitespaces at end of line -- Avoid whitespaces in blank lines - Use {} instead of ; for the empty statement - Single line blocks without { } are allowed, but put braces around multi-line blocks From eb4e3487bc4f850eab5a64dbd309bdde5d2bfb73 Mon Sep 17 00:00:00 2001 From: Peter Schrammel Date: Thu, 8 Dec 2016 11:58:48 +0000 Subject: [PATCH 03/30] Pre-commit hook to run cpplint To install the hook cp .githooks/pre-ommit .git/hooks/pre-commit or use a symbolic link. Then, when running git commit, you should get the linter output (if any) before being prompted to enter a commit message. To bypass the check (e.g. if there was a false positive), add the option --no-verify. --- .githooks/pre-commit | 50 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100755 .githooks/pre-commit diff --git a/.githooks/pre-commit b/.githooks/pre-commit new file mode 100755 index 00000000000..0b2bf51b723 --- /dev/null +++ b/.githooks/pre-commit @@ -0,0 +1,50 @@ +#!/bin/bash +# Runs scripts/cpplint.py on the modified files +# Based on https://github.com/s0enke/git-hooks/ +# +# @author Peter Schrammel + +gitRoot="$(dirname $0)/../.." + +# this is the magic: +# retrieve all files in staging area that are added, modified or renamed +# but no deletions etc +files=$(git diff-index --name-only --cached --diff-filter=ACMR HEAD -- ) + +if [ "$files" == "" ]; then + exit 0 +fi + +# create temporary copy of staging area and delete upon exit +cleanup() +{ + rm -rf $tmpStaging +} + +trap cleanup EXIT + +tmpStaging=$(mktemp -d) + +# Copy contents of staged version of files to temporary staging area +# because we only want the staged version that will be commited and not +# the version in the working directory +stagedFiles="" +for file in $files +do + id=$(git diff-index --cached HEAD $file | cut -d " " -f4) + + # create staged version of file in temporary staging area with the same + # path as the original file + mkdir -p "$tmpStaging/$(dirname $file)" + git cat-file blob $id > "$tmpStaging/$file" + stagedFiles="$stagedFiles $tmpStaging/$file" +done + +output=$(cd $gitRoot; python scripts/cpplint.py $stagedFiles 2>&1) +retval=$? + +if [ $retval -ne 0 ] +then + echo "$output" + exit 1 +fi From ddd871cf6347b1e9a7d9fc9e70cc6bbdb31b5fc2 Mon Sep 17 00:00:00 2001 From: Peter Schrammel Date: Fri, 9 Dec 2016 12:01:36 +0000 Subject: [PATCH 04/30] Fix false alarm on preincrement --- scripts/cpplint.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/cpplint.py b/scripts/cpplint.py index 0389a0e1ef8..48f3ef41a47 100644 --- a/scripts/cpplint.py +++ b/scripts/cpplint.py @@ -3261,7 +3261,7 @@ def CheckOperatorSpacing(filename, clean_lines, linenum, error): # Otherwise not. Note we only check for non-spaces on *both* sides; # sometimes people put non-spaces on one side when aligning ='s among # many lines (not that this is behavior that I approve of...) - match1 = Search(r'[\s](=|>=|<=|==|!=|&=|\^=|\|=|\+=|\*=|\/=|\%=|>|<|\^|\+|\/)', line) + match1 = Search(r'[\s](=|>=|<=|==|!=|&=|\^=|\|=|\+=|\*=|\/=|\%=|>|<|\^|\+[^\+]|\/)', line) match2 = Search(r'(=|>=|<=|==|!=|&=|\^=|\|=|\+=|\*=|\/=|\%=|>|<|!|\^|\+|\/)[\s]', line) if match1 and not Search(r'<<', line) and not Search(r'char \*', line) and not Search(r'\#include', line) and not Search(r'<.*[^\s]>', line): # and not Search(r'\b(if|while|for) ', line) From 50eac162ad411d87d06575ff49c22cf232149b19 Mon Sep 17 00:00:00 2001 From: thk123 Date: Thu, 8 Dec 2016 17:49:06 +0000 Subject: [PATCH 05/30] Fixed linter bug where was thinking catch was a C style cast The linter script has exceptions to exclude incorrect detection of C style casts. Added the keyword catch to this list. Added a regression test to verify this. --- regression/cpp-linter/catch-cast/main.cpp | 12 ++++++++++++ regression/cpp-linter/catch-cast/test.desc | 6 ++++++ scripts/cpplint.py | 2 +- 3 files changed, 19 insertions(+), 1 deletion(-) create mode 100644 regression/cpp-linter/catch-cast/main.cpp create mode 100644 regression/cpp-linter/catch-cast/test.desc diff --git a/regression/cpp-linter/catch-cast/main.cpp b/regression/cpp-linter/catch-cast/main.cpp new file mode 100644 index 00000000000..204d83f935c --- /dev/null +++ b/regression/cpp-linter/catch-cast/main.cpp @@ -0,0 +1,12 @@ +/*******************************************************************\ + +Module: Lint Examples + +Author: Thomas Kiley, thomas@diffblue.com + +\*******************************************************************/ + +static fun() +{ + catch(int) +} diff --git a/regression/cpp-linter/catch-cast/test.desc b/regression/cpp-linter/catch-cast/test.desc new file mode 100644 index 00000000000..63fbf20a5ee --- /dev/null +++ b/regression/cpp-linter/catch-cast/test.desc @@ -0,0 +1,6 @@ +CORE +main.cpp + +^EXIT=0$ +^SIGNAL=0$ +-- diff --git a/scripts/cpplint.py b/scripts/cpplint.py index 48f3ef41a47..ac9f687a48a 100644 --- a/scripts/cpplint.py +++ b/scripts/cpplint.py @@ -5270,7 +5270,7 @@ def CheckCStyleCast(filename, clean_lines, linenum, cast_type, pattern, error): # Exclude lines with keywords that tend to look like casts context = line[0:match.start(1) - 1] - if Match(r'.*\b(?:sizeof|alignof|alignas|[_A-Z][_A-Z0-9]*)\s*$', context): + if Match(r'.*\b(?:sizeof|alignof|alignas|catch|[_A-Z][_A-Z0-9]*)\s*$', context): return False # Try expanding current context to see if we one level of From 9078c92f07b769b3a9ccbb5579a24d6ba2fbd9bd Mon Sep 17 00:00:00 2001 From: thk123 Date: Fri, 16 Dec 2016 18:21:11 +0000 Subject: [PATCH 06/30] Corrected lint error on classes decl that have space after identifier In class declarations, it was previously warnining that the class identifier didn't end with a t because there was a space between the end of the identifier and the : (for inheritance). Now provides a warning that this space shouldn't be present and doesn't warn about the identifier. --- regression/cpp-linter/class-decl-space/main.cpp | 10 ++++++++++ regression/cpp-linter/class-decl-space/test.desc | 7 +++++++ scripts/cpplint.py | 6 +++++- 3 files changed, 22 insertions(+), 1 deletion(-) create mode 100644 regression/cpp-linter/class-decl-space/main.cpp create mode 100644 regression/cpp-linter/class-decl-space/test.desc diff --git a/regression/cpp-linter/class-decl-space/main.cpp b/regression/cpp-linter/class-decl-space/main.cpp new file mode 100644 index 00000000000..97c7db3a835 --- /dev/null +++ b/regression/cpp-linter/class-decl-space/main.cpp @@ -0,0 +1,10 @@ +/*******************************************************************\ + +Module: Lint Examples + +Author: Thomas Kiley, thomas@diffblue.com + +\*******************************************************************/ + +class temp_classt : public base_classt +{} diff --git a/regression/cpp-linter/class-decl-space/test.desc b/regression/cpp-linter/class-decl-space/test.desc new file mode 100644 index 00000000000..87b8d462144 --- /dev/null +++ b/regression/cpp-linter/class-decl-space/test.desc @@ -0,0 +1,7 @@ +CORE +main.cpp + +^main\.cpp:9: There shouldn.t be a space between class identifier and : \[readability/identifiers\] \[4\]$ +^Total errors found: 1$ +^SIGNAL=0$ +-- diff --git a/scripts/cpplint.py b/scripts/cpplint.py index ac9f687a48a..641c4c2c968 100644 --- a/scripts/cpplint.py +++ b/scripts/cpplint.py @@ -3281,8 +3281,12 @@ def CheckOperatorSpacing(filename, clean_lines, linenum, error): error(filename, linenum, 'readability/identifiers', 4, 'Remove spaces around %s' % match.group(0)) +# check any inherited classes don't have a space between the type and the : + if Search(r'(struct|class)\s[\w_]*\s+:', line): + error(filename, linenum, 'readability/identifiers', 4, 'There shouldn\'t be a space between class identifier and :') + #check type definitions end with t - if Search(r'(struct|class)\s[\w_]*[^t^;^:](;$|\s|:|$)', line) and not Search(r'^template <', line) and not Search(r'^template<', line): + if Search(r'(struct|class)\s[\w_]*[^t^;^:^\s](;$|\s|:|$)', line) and not Search(r'^template <', line) and not Search(r'^template<', line): error(filename, linenum, 'readability/identifiers', 4, 'Class or struct identifier should end with t') From 08bdc6e5247a7400a39d80404d240fadc9f4aa8a Mon Sep 17 00:00:00 2001 From: thk123 Date: Fri, 16 Dec 2016 18:34:15 +0000 Subject: [PATCH 07/30] Don't trigger the multi-line parameter warning in comments If the line being tested is a single line comment, we don't bother checking whether the previous line is an incomplete function declaration). --- .../cpp-linter/multi-line-comment/main.cpp | 13 ++++++++++++ .../cpp-linter/multi-line-comment/test.desc | 6 ++++++ scripts/cpplint.py | 21 ++++++++++--------- 3 files changed, 30 insertions(+), 10 deletions(-) create mode 100644 regression/cpp-linter/multi-line-comment/main.cpp create mode 100644 regression/cpp-linter/multi-line-comment/test.desc diff --git a/regression/cpp-linter/multi-line-comment/main.cpp b/regression/cpp-linter/multi-line-comment/main.cpp new file mode 100644 index 00000000000..1e708f42e3d --- /dev/null +++ b/regression/cpp-linter/multi-line-comment/main.cpp @@ -0,0 +1,13 @@ +/*******************************************************************\ + +Module: Lint Examples + +Author: Thomas Kiley, thomas@diffblue.com + +\*******************************************************************/ + +static fun() +{ + // A multi line comment (that includes an opening + // bracket and a closing one on the next line) +} diff --git a/regression/cpp-linter/multi-line-comment/test.desc b/regression/cpp-linter/multi-line-comment/test.desc new file mode 100644 index 00000000000..63fbf20a5ee --- /dev/null +++ b/regression/cpp-linter/multi-line-comment/test.desc @@ -0,0 +1,6 @@ +CORE +main.cpp + +^EXIT=0$ +^SIGNAL=0$ +-- diff --git a/scripts/cpplint.py b/scripts/cpplint.py index 641c4c2c968..d5203a16ad8 100644 --- a/scripts/cpplint.py +++ b/scripts/cpplint.py @@ -4344,16 +4344,17 @@ def CheckStyle(filename, clean_lines, linenum, file_extension, nesting_state, 'Line ends in whitespace. Consider deleting these extra spaces.') # ensure indentation within parenthesized expressions - prev_initial_spaces = 0 - if linenum>0: - while prev_initial_spaces < len(prev) and prev[prev_initial_spaces] == ' ': - prev_initial_spaces += 1 - if Search(r'\([^\)]*,$', line) or Search(r'\(\[^\)]*, $', line): - error(filename, linenum, 'whitespace/indent', 4, - 'If parameters or arguments require a line break, each parameter should be put on its own line.') - if (Search(r'\([^\)]*$', prev) and initial_spaces-2 != prev_initial_spaces) and not Search(r'for|while|if|;', prev): - error(filename, linenum, 'whitespace/indent', 4, - 'Indent of wrapped parenthesized expression or parameter or argument list should be 2') + if (not Search(r'^\s+//', line)): + prev_initial_spaces = 0 + if linenum>0: + while prev_initial_spaces < len(prev) and prev[prev_initial_spaces] == ' ': + prev_initial_spaces += 1 + if Search(r'\([^\)]*,$', line) or Search(r'\(\[^\)]*, $', line): + error(filename, linenum, 'whitespace/indent', 4, + 'If parameters or arguments require a line break, each parameter should be put on its own line.') + if (Search(r'\([^\)]*$', prev) and initial_spaces-2 != prev_initial_spaces) and not Search(r'for|while|if|;', prev): + error(filename, linenum, 'whitespace/indent', 4, + 'Indent of wrapped parenthesized expression or parameter or argument list should be 2') # Check if the line is a header guard. is_header_guard = False From d1af0e1cc2ae62d4230f9f6984e5043733c84a93 Mon Sep 17 00:00:00 2001 From: thk123 Date: Mon, 19 Dec 2016 10:08:20 +0000 Subject: [PATCH 08/30] Added regression test for the prefix iterator in for loop This has been fixed, but previously it would complain about whitespace before the + --- regression/cpp-linter/prefix-increment/main.cpp | 14 ++++++++++++++ regression/cpp-linter/prefix-increment/test.desc | 6 ++++++ 2 files changed, 20 insertions(+) create mode 100644 regression/cpp-linter/prefix-increment/main.cpp create mode 100644 regression/cpp-linter/prefix-increment/test.desc diff --git a/regression/cpp-linter/prefix-increment/main.cpp b/regression/cpp-linter/prefix-increment/main.cpp new file mode 100644 index 00000000000..c63302d19f4 --- /dev/null +++ b/regression/cpp-linter/prefix-increment/main.cpp @@ -0,0 +1,14 @@ +/*******************************************************************\ + +Module: Lint Examples + +Author: Thomas Kiley, thomas@diffblue.com + +\*******************************************************************/ + +static void fun() +{ + for(int i=0; i<10; ++i) + { + } +} diff --git a/regression/cpp-linter/prefix-increment/test.desc b/regression/cpp-linter/prefix-increment/test.desc new file mode 100644 index 00000000000..63fbf20a5ee --- /dev/null +++ b/regression/cpp-linter/prefix-increment/test.desc @@ -0,0 +1,6 @@ +CORE +main.cpp + +^EXIT=0$ +^SIGNAL=0$ +-- From c50e81df0c296463ddcfdfacfa936466edc96309 Mon Sep 17 00:00:00 2001 From: thk123 Date: Mon, 19 Dec 2016 10:09:03 +0000 Subject: [PATCH 09/30] Adding missed make file The make file runs the linter script found in the scripts/ folder on the CPP file found in the test folder. --- regression/cpp-linter/Makefile | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 regression/cpp-linter/Makefile diff --git a/regression/cpp-linter/Makefile b/regression/cpp-linter/Makefile new file mode 100644 index 00000000000..b84f9ae3d18 --- /dev/null +++ b/regression/cpp-linter/Makefile @@ -0,0 +1,20 @@ +default: tests.log + +test: + @if ! ../test.pl -c python ../../../scripts/cpplint.py ; then \ + ../failed-tests-printer.pl ; \ + exit 1 ; \ + fi + +tests.log: ../test.pl + @if ! ../test.pl -c "python ../../../scripts/cpplint.py" ; then \ + ../failed-tests-printer.pl ; \ + exit 1 ; \ + fi + +show: + @for dir in *; do \ + if [ -d "$$dir" ]; then \ + vim -o "$$dir/*.c" "$$dir/*.out"; \ + fi; \ + done; From 19876f5df7c5cbec8e471aa6b3e949a074ed0a84 Mon Sep 17 00:00:00 2001 From: thk123 Date: Mon, 19 Dec 2016 12:11:56 +0000 Subject: [PATCH 10/30] Linter doesn't wrong warn about empty do while statement The linter was picking up that while(...); looks like an empty while statement. However, unlike what the linter is expecting, we put while on a separate line to the closing brace for a do...while statement. Instead the linter now searches previous lines for a matching do statement. If it finds a while before the do then it obviously isn't related to this while. --- regression/cpp-linter/do-while1/main.cpp | 27 +++++++++++++++ regression/cpp-linter/do-while1/test.desc | 8 +++++ scripts/cpplint.py | 40 ++++++++++++++++++++++- 3 files changed, 74 insertions(+), 1 deletion(-) create mode 100644 regression/cpp-linter/do-while1/main.cpp create mode 100644 regression/cpp-linter/do-while1/test.desc diff --git a/regression/cpp-linter/do-while1/main.cpp b/regression/cpp-linter/do-while1/main.cpp new file mode 100644 index 00000000000..dfc0e2442ed --- /dev/null +++ b/regression/cpp-linter/do-while1/main.cpp @@ -0,0 +1,27 @@ +/*******************************************************************\ + +Module: Lint Examples + +Author: Thomas Kiley, thomas@diffblue.com + +\*******************************************************************/ + +static void fun() +{ + // This should not produce a warning + do + { + int x=0; + } + while(a); + + // This should + while(b); + + // As should this + if(true) + { + do_something(); + } + while(c); +} diff --git a/regression/cpp-linter/do-while1/test.desc b/regression/cpp-linter/do-while1/test.desc new file mode 100644 index 00000000000..126809f8684 --- /dev/null +++ b/regression/cpp-linter/do-while1/test.desc @@ -0,0 +1,8 @@ +CORE +main.cpp + +^main\.cpp:19: Empty loop bodies should use {} or continue \[whitespace/empty_loop_body\] \[5\]$ +^main\.cpp:26: Empty loop bodies should use {} or continue \[whitespace/empty_loop_body\] \[5\]$ +^Total errors found: 2$ +^SIGNAL=0$ +-- diff --git a/scripts/cpplint.py b/scripts/cpplint.py index d5203a16ad8..2dfff263042 100644 --- a/scripts/cpplint.py +++ b/scripts/cpplint.py @@ -1611,6 +1611,34 @@ def CloseExpression(clean_lines, linenum, pos): # Did not find end of expression before end of file, give up return (line, clean_lines.NumLines(), -1) +def FindDoStart(clean_lines, linenum): + """If found a while (...); find the potential do to match it. + + Work our way up through the lines starting at linenum to find a do + that hasn't been matched. This might not succeed as might just be an + emtpy while statement. + + Args: + clean_lines: A CleansedLines instance containing the file. + linenum: The line number with the while(...); on + + Returns: + A tuple (found, linenum), found is true if an opening + do statement found, false otherwise. The linenum will + be the line the do is found on (or -1 if never found) + """ + + reverse_lines = clean_lines.lines[linenum-1:0:-1] + found_linenum = linenum - 1; + for line in reverse_lines: + if Search(r'^\s*do\s*{?\s*$', line): + return True, found_linenum + elif Search(r'^\s*}?\s*while\(.*\)\s*;\s*$', line): + return False, -1 + else: + found_linenum = found_linenum - 1 + + return False, -1 def FindStartOfExpressionInLine(line, endpos, stack): """Find position at the matching start of current expression. @@ -3997,7 +4025,17 @@ def CheckEmptyBlockBody(filename, clean_lines, linenum, error): # is likely an error. line = clean_lines.elided[linenum] matched = Match(r'\s*(for|while|if)\s*\(', line) - if matched: + while_matched = Match(r'\s*(while)\s*\(', line) + + # if it is a do while loop, we have the while clause on a separate line + # so need to exclude that + do_found = False + if while_matched: + do_found, num = FindDoStart(clean_lines, linenum) + + is_do_while = while_matched and do_found + # either not a while or if we are, we didn't find a do + if matched and not is_do_while: # Find the end of the conditional expression. (end_line, end_linenum, end_pos) = CloseExpression( clean_lines, linenum, line.find('(')) From 19f29a3fe0f02c3c69e6abe02b29e720a1aedbae Mon Sep 17 00:00:00 2001 From: thk123 Date: Mon, 19 Dec 2016 12:47:29 +0000 Subject: [PATCH 11/30] Check for while is on the same line as closing brace in do-while We check for lines that look like `} while` and see if there is a matching do statement. If there is, we advise the while be put on a separate line. --- regression/cpp-linter/do-while2/main.cpp | 15 +++++++++++++++ regression/cpp-linter/do-while2/test.desc | 7 +++++++ scripts/cpplint.py | 16 ++++++++++++++++ 3 files changed, 38 insertions(+) create mode 100644 regression/cpp-linter/do-while2/main.cpp create mode 100644 regression/cpp-linter/do-while2/test.desc diff --git a/regression/cpp-linter/do-while2/main.cpp b/regression/cpp-linter/do-while2/main.cpp new file mode 100644 index 00000000000..a3bfe00defa --- /dev/null +++ b/regression/cpp-linter/do-while2/main.cpp @@ -0,0 +1,15 @@ +/*******************************************************************\ + +Module: Lint Examples + +Author: Thomas Kiley, thomas@diffblue.com + +\*******************************************************************/ + +static void fun() +{ + do + { + int x=0; + } while(a); +} diff --git a/regression/cpp-linter/do-while2/test.desc b/regression/cpp-linter/do-while2/test.desc new file mode 100644 index 00000000000..1fe4e46ab9b --- /dev/null +++ b/regression/cpp-linter/do-while2/test.desc @@ -0,0 +1,7 @@ +CORE +main.cpp + +^main\.cpp:14: while statement of do...while loop should be on a separate line to the closing brace \[readability/braces\] \[4\] +^Total errors found: 1$ +^SIGNAL=0$ +-- diff --git a/scripts/cpplint.py b/scripts/cpplint.py index 2dfff263042..a96f02beb00 100644 --- a/scripts/cpplint.py +++ b/scripts/cpplint.py @@ -3859,6 +3859,21 @@ def CheckBraces(filename, clean_lines, linenum, error): error(filename, linenum, 'readability/braces', 4, 'If/else bodies with multiple statements require braces') +def CheckDoWhile(filename, clean_lines, linenum, error): + """Looks for while of a do while on the same line as the closing brace + + Args: + filename: The name of the current file. + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + error: The function to call with any errors found. + """ + line = clean_lines.elided[linenum] + if Search(r'}\s*while\s*\(', line): + do_found, num = FindDoStart(clean_lines, linenum) + if do_found: + error(filename, linenum, 'readability/braces', 4, + 'while statement of do...while loop should be on a separate line to the closing brace') def CheckTrailingSemicolon(filename, clean_lines, linenum, error): """Looks for redundant trailing semicolon. @@ -4433,6 +4448,7 @@ def CheckStyle(filename, clean_lines, linenum, file_extension, nesting_state, # Some more style checks CheckBraces(filename, clean_lines, linenum, error) + CheckDoWhile(filename, clean_lines, linenum, error) CheckTrailingSemicolon(filename, clean_lines, linenum, error) CheckEmptyBlockBody(filename, clean_lines, linenum, error) CheckAccess(filename, clean_lines, linenum, nesting_state, error) From cca2e2da88a520f521d94dfd91819bbafc6f2d91 Mon Sep 17 00:00:00 2001 From: thk123 Date: Mon, 19 Dec 2016 18:28:34 +0000 Subject: [PATCH 12/30] Handle template argument lists that run on to multiple lines When checking for spacing around operators, we need to be careful with <, > since are also brackets around template types. In these cases, a space after the > but not before is acceptable. The approach I've taken is to find the matching opening angled bracket and inspect the contents between them. --- regression/cpp-linter/template-types/main.cpp | 24 ++++ .../cpp-linter/template-types/test.desc | 7 + scripts/cpplint.py | 122 +++++++++++++++++- 3 files changed, 151 insertions(+), 2 deletions(-) create mode 100644 regression/cpp-linter/template-types/main.cpp create mode 100644 regression/cpp-linter/template-types/test.desc diff --git a/regression/cpp-linter/template-types/main.cpp b/regression/cpp-linter/template-types/main.cpp new file mode 100644 index 00000000000..a221aadb0f0 --- /dev/null +++ b/regression/cpp-linter/template-types/main.cpp @@ -0,0 +1,24 @@ +/*******************************************************************\ + +Module: Lint Examples + +Author: Thomas Kiley, thomas@diffblue.com + +\*******************************************************************/ + +static void fun() +{ + typedef std::vector number_listt; + std::vector> pairs; + typedef std::vector> instanceof_inst; + + // This isn't a templated type + if(x z) + { + do_something(); + } + + // This is a templated type + std::vector x; +} diff --git a/regression/cpp-linter/template-types/test.desc b/regression/cpp-linter/template-types/test.desc new file mode 100644 index 00000000000..b5d31ac3947 --- /dev/null +++ b/regression/cpp-linter/template-types/test.desc @@ -0,0 +1,7 @@ +CORE +main.cpp + +^main\.cpp:17: Remove spaces around > \[whitespace/operators\] \[4\]$ +^Total errors found: 1$ +^SIGNAL=0$ +-- diff --git a/scripts/cpplint.py b/scripts/cpplint.py index a96f02beb00..6a9c9804f55 100644 --- a/scripts/cpplint.py +++ b/scripts/cpplint.py @@ -1489,6 +1489,104 @@ def _CollapseStrings(elided): return collapsed +def IsTemplateArgumentList_DB(clean_lines, linenum, pos): + """if lines[linenum][pos] is >, finds out if it is closing bracket + of a template argument. + + Finds a matching < (if possible) to the > at position line[linenum][pos] + and then analyzes the string between the two to work out if it could + in fact be a template argument list or just some general code. + + Args: + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + pos: A position on the line. + + Returns: + True if the angled bracket pointed to is the end of a template arguement + list + """ + (start_line, start_linenum, start_pos) = OpenExpression(clean_lines, linenum, pos) + + # We didn't find a matching < so clearly not a template argument list + if start_pos == -1: + return False + + # This collects all the charatcers between the > and matching < + # It also reverses it since we don't care about the actual contents + # we are just looking to make sure no weird characters (like &&) + inbetween_string = "" + if linenum == start_linenum: + inbetween_string = clean_lines.elided[start_linenum][start_pos:pos + 1] + else: + while(start_linenum != linenum): + inbetween_string += clean_lines.elided[start_linenum][start_pos:] + start_linenum += 1 + start_pos = 0 + inbetween_string += clean_lines.elided[linenum][0:pos] + + is_simple_template_params = Match('^[<>(::),\w\s]*$', inbetween_string) + if is_simple_template_params: + return True + + # The operators & or * are accepted within the template arguments + # as we can have reference or pointer types in the list + # however, they must be followed by a > (excepting white space) + # anything else indicates they are being used as a type + + is_looking_for_closing_angle_bracket = False + for char in inbetween_string: + if char in ['&', '*']: + is_looking_for_closing_angle_bracket = True + elif is_looking_for_closing_angle_bracket: + if char == '>': + is_looking_for_closing_angle_bracket = False + elif char not in [' ', '\t']: + return False + + return True + +def OpenExpression(clean_lines, linenum, pos): + """If input points to ) or } or ] or >, finds the position that opens it. + + If lines[linenum][pos] points to a ')' or '}' or ']' or '>', finds the + linenum/pos that correspond to the closing of the expression. + + Essentially a mirror of what CloseExpression does + + TODO(tkiley): could probably be merged with CloseExpression + + Args: + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + pos: A position on the line. + + Returns: + A tuple (line, linenum, pos) pointer *to* the closing brace, or + (line, len(lines), -1) if we never find a close. Note we ignore + strings and comments when matching; and the line we return is the + 'cleansed' line at linenum. + """ + + line = clean_lines.elided[linenum] + if (line[pos] not in ')}]>'): + return (line, clean_lines.NumLines(), -1) + + # Check first line + (end_pos, stack) = FindStartOfExpressionInLine(line, pos , []) + if end_pos > -1: + return (line, linenum, end_pos) + + # Continue scanning forward + while stack and linenum > 0: + linenum -= 1 + line = clean_lines.elided[linenum] + (end_pos, stack) = FindStartOfExpressionInLine(line, len(line) - 1, stack) + if end_pos > -1: + return (line, linenum, end_pos) + + # Did not find end of expression before end of file, give up + return (line, clean_lines.NumLines(), -1) def FindEndOfExpressionInLine(line, startpos, stack): """Find the position just after the end of current parenthesized expression. @@ -2407,6 +2505,13 @@ def InAsmBlock(self): def InTemplateArgumentList(self, clean_lines, linenum, pos): """Check if current position is inside template argument list. + TODO (tkiley): This method seems a little generous for what it + considers to be in a template argument list, providing the line + between pos and the end contains a > (or even an =) before the + end of the line or }, {, ; then it will say yes... For now have + added method IsTemplateArgumentList_DB but should work out + what the idea with this method is a unify. + Args: clean_lines: A CleansedLines instance containing the file. linenum: The number of the line to check. @@ -3291,7 +3396,18 @@ def CheckOperatorSpacing(filename, clean_lines, linenum, error): # many lines (not that this is behavior that I approve of...) match1 = Search(r'[\s](=|>=|<=|==|!=|&=|\^=|\|=|\+=|\*=|\/=|\%=|>|<|\^|\+[^\+]|\/)', line) match2 = Search(r'(=|>=|<=|==|!=|&=|\^=|\|=|\+=|\*=|\/=|\%=|>|<|!|\^|\+|\/)[\s]', line) - if match1 and not Search(r'<<', line) and not Search(r'char \*', line) and not Search(r'\#include', line) and not Search(r'<.*[^\s]>', line): + operator_pos, op_end = match1.span(1) if match1 else (-1, -1) + operator_pos2, op_end2 = match2.span(1) if match2 else (-1, -1) + + if match2 and match2.group(1) == '>': + res = IsTemplateArgumentList_DB(clean_lines, linenum, operator_pos2) + + if (match1 and + not Search(r'<<', line) and # We ignore the left shift operator since might be a stream and then formatting rules go out of the window + not Search(r'char \*', line) and # I don't know why this exception exists? + not Search(r'\#include', line) and # I suppose file names could contains operators?? + #not Search(r'<.*[^\s]>', line)): # This checks for template declarations (providing they don't run onto next line) + not IsTemplateArgumentList_DB(clean_lines, linenum, operator_pos)): # and not Search(r'\b(if|while|for) ', line) # # Operators taken from [lex.operators] in C++11 standard. # and not Search(r'(>=|<=|==|!=|&=|\^=|\|=|\+=|\*=|\/=|\%=)', line) @@ -3299,7 +3415,9 @@ def CheckOperatorSpacing(filename, clean_lines, linenum, error): error(filename, linenum, 'whitespace/operators', 4, # 'Missing spaces around =') 'Remove spaces around %s' % match1.group(1)) - elif match2 and not Search(r'<<', line) and not Search(r'<[^\s].*>', line): + elif (match2 and + not Search(r'<<', line) and + not IsTemplateArgumentList_DB(clean_lines, linenum, operator_pos2)): error(filename, linenum, 'whitespace/operators', 4, 'Remove spaces around %s' % match2.group(0)) From 0a1383cf4b2fd42f87eda0cde82482f543519b61 Mon Sep 17 00:00:00 2001 From: thk123 Date: Tue, 20 Dec 2016 14:05:32 +0000 Subject: [PATCH 13/30] Removed code that wasn't being used The if and the prevline weren't commented out, but since the variable wasn't being used it was just making it hard to identify the scope of the if statement. --- scripts/cpplint.py | 24 +----------------------- 1 file changed, 1 insertion(+), 23 deletions(-) diff --git a/scripts/cpplint.py b/scripts/cpplint.py index 6a9c9804f55..6815bc4b099 100644 --- a/scripts/cpplint.py +++ b/scripts/cpplint.py @@ -3871,29 +3871,7 @@ def CheckBraces(filename, clean_lines, linenum, error): line = clean_lines.elided[linenum] # get rid of comments and strings - if Match(r'\s*{\s*$', line): - # We allow an open brace to start a line in the case where someone is using - # braces in a block to explicitly create a new scope, which is commonly used - # to control the lifetime of stack-allocated variables. Braces are also - # used for brace initializers inside function calls. We don't detect this - # perfectly: we just don't complain if the last non-whitespace character on - # the previous non-blank line is ',', ';', ':', '(', '{', or '}', or if the - # previous line starts a preprocessor block. We also allow a brace on the - # following line if it is part of an array initialization and would not fit - # within the 80 character limit of the preceding line. - prevline = GetPreviousNonBlankLine(clean_lines, linenum)[0] -# if (not Search(r'[,;:}{(]\s*$', prevline) and -# not Match(r'\s*#', prevline) and -# not (GetLineWidth(prevline) > _line_length - 2 and '[]' in prevline)): -# error(filename, linenum, 'whitespace/braces', 4, -# '{ should almost always be at the end of the previous line') - - # An else clause should be on the same line as the preceding closing brace. -# if Match(r'\s*else\b\s*(?:if\b|\{|$)', line): -# prevline = GetPreviousNonBlankLine(clean_lines, linenum)[0] -# if Match(r'\s*}\s*$', prevline): -# error(filename, linenum, 'whitespace/newline', 4, -# 'An else should appear on the same line as the preceding }') + # If braces come on one side of an else, they should be on both. # However, we have to worry about "else if" that spans multiple lines! From 773cdb7aca096f25df0dcf32bdad3417dd20935a Mon Sep 17 00:00:00 2001 From: thk123 Date: Tue, 20 Dec 2016 14:06:11 +0000 Subject: [PATCH 14/30] Else statement with brace on same line Tidied up the if/else if/else errors (needs more tests) --- regression/cpp-linter/if-else1/main.cpp | 19 ++++++++++++++++ regression/cpp-linter/if-else1/test.desc | 11 ++++++++++ scripts/cpplint.py | 28 ++++++++++++++++++++++++ 3 files changed, 58 insertions(+) create mode 100644 regression/cpp-linter/if-else1/main.cpp create mode 100644 regression/cpp-linter/if-else1/test.desc diff --git a/regression/cpp-linter/if-else1/main.cpp b/regression/cpp-linter/if-else1/main.cpp new file mode 100644 index 00000000000..43d24bc47cd --- /dev/null +++ b/regression/cpp-linter/if-else1/main.cpp @@ -0,0 +1,19 @@ +/*******************************************************************\ + +Module: Lint Examples + +Author: Thomas Kiley, thomas@diffblue.com + +\*******************************************************************/ + +static void fun() +{ + bool condition=false; + if(condition) { + fun(); + } else if(condition) { + fun(); + } else { + fun(); + } +} diff --git a/regression/cpp-linter/if-else1/test.desc b/regression/cpp-linter/if-else1/test.desc new file mode 100644 index 00000000000..a32899d0df0 --- /dev/null +++ b/regression/cpp-linter/if-else1/test.desc @@ -0,0 +1,11 @@ +CORE +main.cpp + +^main\.cpp:12: Put braces on a separate next line \[whitespace/braces\] \[5\]$ +^main\.cpp:14: Put braces on a separate next line \[whitespace/braces\] \[5\]$ +^main\.cpp:14: Else if should be on a new line after closing brace \[readability/braces\] \[5\]$ +^main\.cpp:16: Put braces on a separate next line \[whitespace/braces\] \[5\]$ +^main\.cpp:16: Else should be on a new line after closing brace \[readability/braces\] \[5\]$ +^Total errors found: 5$ +^SIGNAL=0$ +-- diff --git a/scripts/cpplint.py b/scripts/cpplint.py index 6815bc4b099..1d5d6ad52bd 100644 --- a/scripts/cpplint.py +++ b/scripts/cpplint.py @@ -3871,7 +3871,35 @@ def CheckBraces(filename, clean_lines, linenum, error): line = clean_lines.elided[linenum] # get rid of comments and strings + # Check for if/else if/else that have brace on the same line + if Search(r'if\(.*\)\s*{', line): + error(filename, linenum, 'readability/braces', 5, + 'Brace after an if should be on a new line') + elif Search(r'else if\(.*\)\s*{', line): + error(filename, linenum, 'readability/braces', 5, + 'Brace after else if should be on a new line') + elif Search(r'else\s*{', line): + error(filename, linenum, 'readability/braces', 5, + 'Brace after else should be on a new line') + # Check for else if/else that are on the same line as the previous brace + elif Search(r'}\s*else if', line): + error(filename, linenum, 'readability/braces', 5, + 'Else if should be on a new line after closing brace') + elif Search(r'}\s*else', line): + error(filename, linenum, 'readability/braces', 5, + 'Else should be on a new line after closing brace') + + # Check for if/else if/else don't have statements on the same line + if Search(r'if\(.*\)\s*[^{\s]+', line): + error(filename, linenum, 'readability/braces', 5, + 'Statement after an if should be on a new line') + elif Search(r'else if\(.*\)\s*[^{]+', line): + error(filename, linenum, 'readability/braces', 5, + 'Statement after else if should be on a new line') + elif Search(r'else\s[^{]+', line): + error(filename, linenum, 'readability/braces', 5, + 'Statement after else should be on a new line') # If braces come on one side of an else, they should be on both. # However, we have to worry about "else if" that spans multiple lines! From df109bc5aeedaef94c0287385fac7b08136bdb73 Mon Sep 17 00:00:00 2001 From: thk123 Date: Tue, 20 Dec 2016 16:03:30 +0000 Subject: [PATCH 15/30] Further developments on if lint checking Removed most of the old lint if checking as they want braces on the same line so was mostly the opposite of what we wanted (which is in fact much simpler). Removed the old check for if(...) { as this is already picked up by the braces at the end of line checks. Added additional tests for different ways the if statement can be wrong or right. --- regression/cpp-linter/if-else2/main.cpp | 20 +++++++++++ regression/cpp-linter/if-else2/test.desc | 9 +++++ regression/cpp-linter/if-else3/main.cpp | 15 ++++++++ regression/cpp-linter/if-else3/test.desc | 9 +++++ regression/cpp-linter/if-else4/main.cpp | 15 ++++++++ regression/cpp-linter/if-else4/test.desc | 8 +++++ regression/cpp-linter/if-else5/main.cpp | 24 +++++++++++++ regression/cpp-linter/if-else5/test.desc | 7 ++++ regression/cpp-linter/if-else6/main.cpp | 18 ++++++++++ regression/cpp-linter/if-else6/test.desc | 7 ++++ scripts/cpplint.py | 46 ++++-------------------- 11 files changed, 139 insertions(+), 39 deletions(-) create mode 100644 regression/cpp-linter/if-else2/main.cpp create mode 100644 regression/cpp-linter/if-else2/test.desc create mode 100644 regression/cpp-linter/if-else3/main.cpp create mode 100644 regression/cpp-linter/if-else3/test.desc create mode 100644 regression/cpp-linter/if-else4/main.cpp create mode 100644 regression/cpp-linter/if-else4/test.desc create mode 100644 regression/cpp-linter/if-else5/main.cpp create mode 100644 regression/cpp-linter/if-else5/test.desc create mode 100644 regression/cpp-linter/if-else6/main.cpp create mode 100644 regression/cpp-linter/if-else6/test.desc diff --git a/regression/cpp-linter/if-else2/main.cpp b/regression/cpp-linter/if-else2/main.cpp new file mode 100644 index 00000000000..c4c2d84b38c --- /dev/null +++ b/regression/cpp-linter/if-else2/main.cpp @@ -0,0 +1,20 @@ +/*******************************************************************\ + +Module: Lint Examples + +Author: Thomas Kiley, thomas@diffblue.com + +\*******************************************************************/ + +static void fun() +{ + if(condition) { + fun(); + } + else if(condition) { + fun(); + } + else { + fun(); + } +} diff --git a/regression/cpp-linter/if-else2/test.desc b/regression/cpp-linter/if-else2/test.desc new file mode 100644 index 00000000000..ef4d2be01e9 --- /dev/null +++ b/regression/cpp-linter/if-else2/test.desc @@ -0,0 +1,9 @@ +CORE +main.cpp + +^main\.cpp:11: Put braces on a separate next line \[whitespace/braces\] \[5\]$ +^main\.cpp:14: Put braces on a separate next line \[whitespace/braces\] \[5\]$ +^main\.cpp:17: Put braces on a separate next line \[whitespace/braces\] \[5\]$ +^Total errors found: 3$ +^SIGNAL=0$ +-- diff --git a/regression/cpp-linter/if-else3/main.cpp b/regression/cpp-linter/if-else3/main.cpp new file mode 100644 index 00000000000..035be757c17 --- /dev/null +++ b/regression/cpp-linter/if-else3/main.cpp @@ -0,0 +1,15 @@ +/*******************************************************************\ + +Module: Lint Examples + +Author: Thomas Kiley, thomas@diffblue.com + +\*******************************************************************/ + +static void fun() +{ + bool condition=false; + if(condition) fun(); + else if(condition) fun(); + else fun(); +} diff --git a/regression/cpp-linter/if-else3/test.desc b/regression/cpp-linter/if-else3/test.desc new file mode 100644 index 00000000000..0a0073495ad --- /dev/null +++ b/regression/cpp-linter/if-else3/test.desc @@ -0,0 +1,9 @@ +CORE +main.cpp + +^main\.cpp:12: Statement after an if should be on a new line \[readability/braces\] \[5\]$ +^main\.cpp:13: Statement after else if should be on a new line \[readability/braces\] \[5\]$ +^main\.cpp:14: Statement after else should be on a new line \[readability/braces\] \[5\]$ +^Total errors found: 3$ +^SIGNAL=0$ +-- diff --git a/regression/cpp-linter/if-else4/main.cpp b/regression/cpp-linter/if-else4/main.cpp new file mode 100644 index 00000000000..0d599dd0e36 --- /dev/null +++ b/regression/cpp-linter/if-else4/main.cpp @@ -0,0 +1,15 @@ +/*******************************************************************\ + +Module: Lint Examples + +Author: Thomas Kiley, thomas@diffblue.com + +\*******************************************************************/ + +static void fun() +{ + bool condition=false; + if(condition) { fun(); } + else if(condition) { fun(); } + else { fun(); } +} diff --git a/regression/cpp-linter/if-else4/test.desc b/regression/cpp-linter/if-else4/test.desc new file mode 100644 index 00000000000..8aac427632a --- /dev/null +++ b/regression/cpp-linter/if-else4/test.desc @@ -0,0 +1,8 @@ +CORE +main.cpp + +^main\.cpp:12: Statement after an if should be on a new line \[readability/braces\] \[5\]$ +^main\.cpp:13: Statement after else if should be on a new line \[readability/braces\] \[5\]$ +^Total errors found: 3$ +^SIGNAL=0$ +-- diff --git a/regression/cpp-linter/if-else5/main.cpp b/regression/cpp-linter/if-else5/main.cpp new file mode 100644 index 00000000000..a635b257c38 --- /dev/null +++ b/regression/cpp-linter/if-else5/main.cpp @@ -0,0 +1,24 @@ +/*******************************************************************\ + +Module: Lint Examples + +Author: Thomas Kiley, thomas@diffblue.com + +\*******************************************************************/ + +static void fun() +{ + bool condition=false; + if(condition) + { + fun(); + } + else if(condition) + { + fun(); + } + else + { + fun(); + } +} diff --git a/regression/cpp-linter/if-else5/test.desc b/regression/cpp-linter/if-else5/test.desc new file mode 100644 index 00000000000..12418d892f4 --- /dev/null +++ b/regression/cpp-linter/if-else5/test.desc @@ -0,0 +1,7 @@ +CORE +main.cpp + +^Total errors found: 0$ +^EXIT=0$ +^SIGNAL=0$ +-- diff --git a/regression/cpp-linter/if-else6/main.cpp b/regression/cpp-linter/if-else6/main.cpp new file mode 100644 index 00000000000..ae62578a440 --- /dev/null +++ b/regression/cpp-linter/if-else6/main.cpp @@ -0,0 +1,18 @@ +/*******************************************************************\ + +Module: Lint Examples + +Author: Thomas Kiley, thomas@diffblue.com + +\*******************************************************************/ + +static void fun() +{ + bool condition=false; + if(condition) + fun(); + else if(condition) + fun(); + else + fun(); +} diff --git a/regression/cpp-linter/if-else6/test.desc b/regression/cpp-linter/if-else6/test.desc new file mode 100644 index 00000000000..12418d892f4 --- /dev/null +++ b/regression/cpp-linter/if-else6/test.desc @@ -0,0 +1,7 @@ +CORE +main.cpp + +^Total errors found: 0$ +^EXIT=0$ +^SIGNAL=0$ +-- diff --git a/scripts/cpplint.py b/scripts/cpplint.py index 1d5d6ad52bd..3e45a1cacc9 100644 --- a/scripts/cpplint.py +++ b/scripts/cpplint.py @@ -3871,19 +3871,8 @@ def CheckBraces(filename, clean_lines, linenum, error): line = clean_lines.elided[linenum] # get rid of comments and strings - # Check for if/else if/else that have brace on the same line - if Search(r'if\(.*\)\s*{', line): - error(filename, linenum, 'readability/braces', 5, - 'Brace after an if should be on a new line') - elif Search(r'else if\(.*\)\s*{', line): - error(filename, linenum, 'readability/braces', 5, - 'Brace after else if should be on a new line') - elif Search(r'else\s*{', line): - error(filename, linenum, 'readability/braces', 5, - 'Brace after else should be on a new line') - # Check for else if/else that are on the same line as the previous brace - elif Search(r'}\s*else if', line): + if Search(r'}\s*else if', line): error(filename, linenum, 'readability/braces', 5, 'Else if should be on a new line after closing brace') elif Search(r'}\s*else', line): @@ -3891,38 +3880,17 @@ def CheckBraces(filename, clean_lines, linenum, error): 'Else should be on a new line after closing brace') # Check for if/else if/else don't have statements on the same line - if Search(r'if\(.*\)\s*[^{\s]+', line): + if Search(r'^\s*if\(.*\)\s*\s*{?\s*.+;', line): error(filename, linenum, 'readability/braces', 5, 'Statement after an if should be on a new line') - elif Search(r'else if\(.*\)\s*[^{]+', line): - error(filename, linenum, 'readability/braces', 5, - 'Statement after else if should be on a new line') - elif Search(r'else\s[^{]+', line): + elif Search(r'else if\(.*\)', line): + if Search(r'else if\(.*\)\s*{?\s*.+;', line): + error(filename, linenum, 'readability/braces', 5, + 'Statement after else if should be on a new line') + elif Search(r'else\s*{?\s*.+;', line): error(filename, linenum, 'readability/braces', 5, 'Statement after else should be on a new line') - # If braces come on one side of an else, they should be on both. - # However, we have to worry about "else if" that spans multiple lines! - if Search(r'else if\s*\(', line): # could be multi-line if - brace_on_left = bool(Search(r'}\s*else if\s*\(', line)) - # find the ( after the if - pos = line.find('else if') - pos = line.find('(', pos) - if pos > 0: - (endline, _, endpos) = CloseExpression(clean_lines, linenum, pos) - brace_on_right = endline[endpos:].find('{') != -1 - if brace_on_left != brace_on_right: # must be brace after if - error(filename, linenum, 'readability/braces', 5, - 'If an else has a brace on one side, it should have it on both') - elif Search(r'}\s*else[^{]*$', line) or Match(r'[^}]*else\s*{', line): - error(filename, linenum, 'readability/braces', 5, - 'If an else has a brace on one side, it should have it on both') - - # Likewise, an else should never have the else clause on the same line - if Search(r'\belse [^\s{]', line) and not Search(r'\belse if\b', line): - error(filename, linenum, 'whitespace/newline', 4, - 'Else clause should never be on same line as else (use 2 lines)') - # In the same way, a do/while should never be on one line if Match(r'\s*do [^\s{]', line): error(filename, linenum, 'whitespace/newline', 4, From edcb06ad505f2a1bd17ffb3507461938fc0f5d2c Mon Sep 17 00:00:00 2001 From: thk123 Date: Tue, 20 Dec 2016 16:04:31 +0000 Subject: [PATCH 16/30] Fixing whitespace errors in the script --- scripts/cpplint.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/cpplint.py b/scripts/cpplint.py index 3e45a1cacc9..35f75602943 100644 --- a/scripts/cpplint.py +++ b/scripts/cpplint.py @@ -169,8 +169,8 @@ The "root" option is similar in function to the --root flag (see example above). - - The "headers" option is similar in function to the --headers flag + + The "headers" option is similar in function to the --headers flag (see example above). CPPLINT.cfg has an effect on files in the same directory and all @@ -4483,7 +4483,7 @@ def CheckStyle(filename, clean_lines, linenum, file_extension, nesting_state, error(filename, linenum, 'whitespace/indent', 3, 'Weird number of spaces at line-start. ' 'Are you using a 2-space indent?') - + if line and line[-1].isspace(): error(filename, linenum, 'whitespace/end_of_line', 4, 'Line ends in whitespace. Consider deleting these extra spaces.') From e6a50931e2a8592c4cef94562f19934be5f330a6 Mon Sep 17 00:00:00 2001 From: thk123 Date: Tue, 20 Dec 2016 16:13:07 +0000 Subject: [PATCH 17/30] Disabled test for redundant override final specifier --- regression/cpp-linter/override-final/main.cpp | 13 ++++++++ .../cpp-linter/override-final/test.desc | 6 ++++ scripts/cpplint.py | 31 ------------------- 3 files changed, 19 insertions(+), 31 deletions(-) create mode 100644 regression/cpp-linter/override-final/main.cpp create mode 100644 regression/cpp-linter/override-final/test.desc diff --git a/regression/cpp-linter/override-final/main.cpp b/regression/cpp-linter/override-final/main.cpp new file mode 100644 index 00000000000..455a029f774 --- /dev/null +++ b/regression/cpp-linter/override-final/main.cpp @@ -0,0 +1,13 @@ +/*******************************************************************\ + +Module: Lint Examples + +Author: Thomas Kiley, thomas@diffblue.com + +\*******************************************************************/ + +class test_classt:public base_classt +{ +public: + virtual void fun() override final; +}; diff --git a/regression/cpp-linter/override-final/test.desc b/regression/cpp-linter/override-final/test.desc new file mode 100644 index 00000000000..63fbf20a5ee --- /dev/null +++ b/regression/cpp-linter/override-final/test.desc @@ -0,0 +1,6 @@ +CORE +main.cpp + +^EXIT=0$ +^SIGNAL=0$ +-- diff --git a/scripts/cpplint.py b/scripts/cpplint.py index 35f75602943..f4420436f7d 100644 --- a/scripts/cpplint.py +++ b/scripts/cpplint.py @@ -5812,36 +5812,6 @@ def CheckRedundantVirtual(filename, clean_lines, linenum, error): # break -def CheckRedundantOverrideOrFinal(filename, clean_lines, linenum, error): - """Check if line contains a redundant "override" or "final" virt-specifier. - - Args: - filename: The name of the current file. - clean_lines: A CleansedLines instance containing the file. - linenum: The number of the line to check. - error: The function to call with any errors found. - """ - # Look for closing parenthesis nearby. We need one to confirm where - # the declarator ends and where the virt-specifier starts to avoid - # false positives. - line = clean_lines.elided[linenum] - declarator_end = line.rfind(')') - if declarator_end >= 0: - fragment = line[declarator_end:] - else: - if linenum > 1 and clean_lines.elided[linenum - 1].rfind(')') >= 0: - fragment = line - else: - return - - # Check that at most one of "override" or "final" is present, not both - if Search(r'\boverride\b', fragment) and Search(r'\bfinal\b', fragment): - error(filename, linenum, 'readability/inheritance', 4, - ('"override" is redundant since function is ' - 'already declared as "final"')) - - - # Returns true if we are at a new block, and it is directly # inside of a namespace. @@ -5955,7 +5925,6 @@ def ProcessLine(filename, file_extension, clean_lines, line, CheckInvalidIncrement(filename, clean_lines, line, error) CheckMakePairUsesDeduction(filename, clean_lines, line, error) CheckRedundantVirtual(filename, clean_lines, line, error) - CheckRedundantOverrideOrFinal(filename, clean_lines, line, error) CheckNamespaceOrUsing(filename, clean_lines, line, error) for check_fn in extra_check_functions: check_fn(filename, clean_lines, line, error) From 13b7639d55abcbc5211abfc44d4e62a272f49b0f Mon Sep 17 00:00:00 2001 From: thk123 Date: Tue, 20 Dec 2016 16:51:22 +0000 Subject: [PATCH 18/30] Don't demand removing space if operator starts a line To deal with splitting lines containing operators up, if the operator starts the new line, we shouldn't require white space before it be removed. Instead, we only check for white space on the left side of operators if we've encountered some non-white-space characters that line. There are about 500 cases in the code where the operator starts the new line and 250 where it ends the previous line so not practical to require one over the other. --- .../cpp-linter/multi-line-string1/main.cpp | 18 ++++++++++++++++++ .../cpp-linter/multi-line-string1/test.desc | 6 ++++++ scripts/cpplint.py | 2 +- 3 files changed, 25 insertions(+), 1 deletion(-) create mode 100644 regression/cpp-linter/multi-line-string1/main.cpp create mode 100644 regression/cpp-linter/multi-line-string1/test.desc diff --git a/regression/cpp-linter/multi-line-string1/main.cpp b/regression/cpp-linter/multi-line-string1/main.cpp new file mode 100644 index 00000000000..436fe52a8db --- /dev/null +++ b/regression/cpp-linter/multi-line-string1/main.cpp @@ -0,0 +1,18 @@ +/*******************************************************************\ + +Module: Lint Examples + +Author: Thomas Kiley, thomas@diffblue.com + +\*******************************************************************/ + +static void fun() +{ + auto fn_prettyname=id2string(arg0.get(ID_C_class)) + .substr(java_prefix.size()) + +"."+id2string(fn_basename)+"()"; + + auto fn_prettyname=id2string(arg0.get(ID_C_class)) + .substr(java_prefix.size())+ + "."+id2string(fn_basename)+"()"; +} diff --git a/regression/cpp-linter/multi-line-string1/test.desc b/regression/cpp-linter/multi-line-string1/test.desc new file mode 100644 index 00000000000..63fbf20a5ee --- /dev/null +++ b/regression/cpp-linter/multi-line-string1/test.desc @@ -0,0 +1,6 @@ +CORE +main.cpp + +^EXIT=0$ +^SIGNAL=0$ +-- diff --git a/scripts/cpplint.py b/scripts/cpplint.py index f4420436f7d..448f05fbe73 100644 --- a/scripts/cpplint.py +++ b/scripts/cpplint.py @@ -3394,7 +3394,7 @@ def CheckOperatorSpacing(filename, clean_lines, linenum, error): # Otherwise not. Note we only check for non-spaces on *both* sides; # sometimes people put non-spaces on one side when aligning ='s among # many lines (not that this is behavior that I approve of...) - match1 = Search(r'[\s](=|>=|<=|==|!=|&=|\^=|\|=|\+=|\*=|\/=|\%=|>|<|\^|\+[^\+]|\/)', line) + match1 = Search(r'[^\s]+[\s](=|>=|<=|==|!=|&=|\^=|\|=|\+=|\*=|\/=|\%=|>|<|\^|\+[^\+]|\/)', line) match2 = Search(r'(=|>=|<=|==|!=|&=|\^=|\|=|\+=|\*=|\/=|\%=|>|<|!|\^|\+|\/)[\s]', line) operator_pos, op_end = match1.span(1) if match1 else (-1, -1) operator_pos2, op_end2 = match2.span(1) if match2 else (-1, -1) From 4f803c604e42b3b8e2285b962a37b388eaa65355 Mon Sep 17 00:00:00 2001 From: thk123 Date: Tue, 20 Dec 2016 17:58:07 +0000 Subject: [PATCH 19/30] Added test for spacing error that wasn't being triggered --- regression/cpp-linter/operator-spacing1/main.cpp | 12 ++++++++++++ regression/cpp-linter/operator-spacing1/test.desc | 7 +++++++ 2 files changed, 19 insertions(+) create mode 100644 regression/cpp-linter/operator-spacing1/main.cpp create mode 100644 regression/cpp-linter/operator-spacing1/test.desc diff --git a/regression/cpp-linter/operator-spacing1/main.cpp b/regression/cpp-linter/operator-spacing1/main.cpp new file mode 100644 index 00000000000..eebfb7cb5b3 --- /dev/null +++ b/regression/cpp-linter/operator-spacing1/main.cpp @@ -0,0 +1,12 @@ +/*******************************************************************\ + +Module: Lint Examples + +Author: Thomas Kiley, thomas@diffblue.com + +\*******************************************************************/ + +static void fun() +{ + version_set = true; +} diff --git a/regression/cpp-linter/operator-spacing1/test.desc b/regression/cpp-linter/operator-spacing1/test.desc new file mode 100644 index 00000000000..67b95af2d0b --- /dev/null +++ b/regression/cpp-linter/operator-spacing1/test.desc @@ -0,0 +1,7 @@ +CORE +main.cpp + +^main\.cpp:11: Remove spaces around = \[whitespace/operators\] \[4\] +^Total errors found: 1$ +^SIGNAL=0$ +-- From 76193a2a23fc9c95f0aeaba64d7da1c83560ad7b Mon Sep 17 00:00:00 2001 From: thk123 Date: Tue, 20 Dec 2016 18:17:54 +0000 Subject: [PATCH 20/30] Use elided lines when checking bracket indentation This fixes previous bug with brackets in comments (in a cleaner and more consistent way) as well as addressing the bug to do with brackets in comments. Weirdly, it was claiming to be using lines with strings removed, but printing the line clearly shows that `lines_without_raw_strings` still contains at least some strings. --- .../cpp-linter/multi-line-string2/main.cpp | 13 ++++++++++ .../cpp-linter/multi-line-string2/test.desc | 6 +++++ scripts/cpplint.py | 26 ++++++++++--------- 3 files changed, 33 insertions(+), 12 deletions(-) create mode 100644 regression/cpp-linter/multi-line-string2/main.cpp create mode 100644 regression/cpp-linter/multi-line-string2/test.desc diff --git a/regression/cpp-linter/multi-line-string2/main.cpp b/regression/cpp-linter/multi-line-string2/main.cpp new file mode 100644 index 00000000000..73f0fa52adc --- /dev/null +++ b/regression/cpp-linter/multi-line-string2/main.cpp @@ -0,0 +1,13 @@ +/*******************************************************************\ + +Module: Lint Examples + +Author: Thomas Kiley, thomas@diffblue.com + +\*******************************************************************/ + +static void fun() +{ + status() << "Adding CPROVER library (" + << config.ansi_c.arch << ")" << eom; +} diff --git a/regression/cpp-linter/multi-line-string2/test.desc b/regression/cpp-linter/multi-line-string2/test.desc new file mode 100644 index 00000000000..63fbf20a5ee --- /dev/null +++ b/regression/cpp-linter/multi-line-string2/test.desc @@ -0,0 +1,6 @@ +CORE +main.cpp + +^EXIT=0$ +^SIGNAL=0$ +-- diff --git a/scripts/cpplint.py b/scripts/cpplint.py index 448f05fbe73..efe8d9d371a 100644 --- a/scripts/cpplint.py +++ b/scripts/cpplint.py @@ -4448,6 +4448,9 @@ def CheckStyle(filename, clean_lines, linenum, file_extension, nesting_state, line = raw_lines[linenum] prev = raw_lines[linenum - 1] if linenum > 0 else '' + elided_line = clean_lines.elided[linenum] + elided_prev = clean_lines.elided[linenum - 1] if linenum > 0 else '' + if line.find('\t') != -1: error(filename, linenum, 'whitespace/tab', 1, 'Tab found, replace by spaces') @@ -4488,18 +4491,17 @@ def CheckStyle(filename, clean_lines, linenum, file_extension, nesting_state, error(filename, linenum, 'whitespace/end_of_line', 4, 'Line ends in whitespace. Consider deleting these extra spaces.') -# ensure indentation within parenthesized expressions - if (not Search(r'^\s+//', line)): - prev_initial_spaces = 0 - if linenum>0: - while prev_initial_spaces < len(prev) and prev[prev_initial_spaces] == ' ': - prev_initial_spaces += 1 - if Search(r'\([^\)]*,$', line) or Search(r'\(\[^\)]*, $', line): - error(filename, linenum, 'whitespace/indent', 4, - 'If parameters or arguments require a line break, each parameter should be put on its own line.') - if (Search(r'\([^\)]*$', prev) and initial_spaces-2 != prev_initial_spaces) and not Search(r'for|while|if|;', prev): - error(filename, linenum, 'whitespace/indent', 4, - 'Indent of wrapped parenthesized expression or parameter or argument list should be 2') +# ensure indentation within parenthesized expressions (only on elided lines since we don't care about comments or strings) + prev_initial_spaces = 0 + if linenum>0: + while prev_initial_spaces < len(prev) and prev[prev_initial_spaces] == ' ': + prev_initial_spaces += 1 + if Search(r'\([^\)]*,$', elided_line) or Search(r'\(\[^\)]*, $', elided_line): + error(filename, linenum, 'whitespace/indent', 4, + 'If parameters or arguments require a line break, each parameter should be put on its own line.') + if (Search(r'\([^\)]*$', elided_prev) and initial_spaces-2 != prev_initial_spaces) and not Search(r'for|while|if|;', elided_prev): + error(filename, linenum, 'whitespace/indent', 4, + 'Indent of wrapped parenthesized expression or parameter or argument list should be 2') # Check if the line is a header guard. is_header_guard = False From 60cb406822d68897d068b820055d2c19fa3da2fc Mon Sep 17 00:00:00 2001 From: thk123 Date: Tue, 20 Dec 2016 18:30:24 +0000 Subject: [PATCH 21/30] Don't warn about spacing around << as difficult to know if we should Since we can't in general (without type analysis I think) tell the difference between a bit shift operator << and a stream manipulator << we don't warn about it. In some instances we can tell if it is a stream manipulator so we check for spaces there. I've added a known bug operator-spacing3 that would be the perfect report of errors. --- .../cpp-linter/operator-spacing2/main.cpp | 19 +++++++++++++++++++ .../cpp-linter/operator-spacing2/test.desc | 7 +++++++ .../cpp-linter/operator-spacing3/main.cpp | 18 ++++++++++++++++++ .../cpp-linter/operator-spacing3/test.desc | 10 ++++++++++ scripts/cpplint.py | 2 +- 5 files changed, 55 insertions(+), 1 deletion(-) create mode 100644 regression/cpp-linter/operator-spacing2/main.cpp create mode 100644 regression/cpp-linter/operator-spacing2/test.desc create mode 100644 regression/cpp-linter/operator-spacing3/main.cpp create mode 100644 regression/cpp-linter/operator-spacing3/test.desc diff --git a/regression/cpp-linter/operator-spacing2/main.cpp b/regression/cpp-linter/operator-spacing2/main.cpp new file mode 100644 index 00000000000..c11967d0eb0 --- /dev/null +++ b/regression/cpp-linter/operator-spacing2/main.cpp @@ -0,0 +1,19 @@ +/*******************************************************************\ + +Module: Lint Examples + +Author: Thomas Kiley, thomas@diffblue.com + +\*******************************************************************/ + +static void fun() +{ + status() << "Adding CPROVER library (" << eom; + + int x = 1<<4; + + status()<<"Adding CPROVER library ("<=!\s](==|!=|<=|>=|\|\|)[^<>=!\s,;\)]', line) - match = Search(r'[^\s](<<|&&|\|\|)[^\s]', line) + match = Search(r'[^\s](&&|\|\|)[^\s]', line) if match: error(filename, linenum, 'whitespace/operators', 3, 'Missing spaces around %s' % match.group(1)) From e24f0455493f7037dc7ef94782778deb5d8f1f91 Mon Sep 17 00:00:00 2001 From: thk123 Date: Wed, 21 Dec 2016 13:36:00 +0000 Subject: [PATCH 22/30] Lint script checks for the function comment header Each function with a body should have a function comment header directly above it. Modified the lint script to check for its presence and validate that it meeds certain requirements. Updated existing tests to include the funciton header so they don't generate warnings for its absence. Also updated the test description as the line numbers of the deliberate errors have changed. Added new tests for the different ways the function header can be wrong --- regression/cpp-linter/catch-cast/main.cpp | 14 +- regression/cpp-linter/do-while1/main.cpp | 12 ++ regression/cpp-linter/do-while1/test.desc | 4 +- regression/cpp-linter/do-while2/main.cpp | 12 ++ regression/cpp-linter/do-while2/test.desc | 2 +- .../function-comment-header1/main.cpp | 30 ++++ .../function-comment-header1/test.desc | 8 + .../function-comment-header2/main.cpp | 18 +++ .../function-comment-header2/test.desc | 9 ++ .../function-comment-header3/main.cpp | 23 +++ .../function-comment-header3/test.desc | 7 + .../function-comment-header4/main.cpp | 24 +++ .../function-comment-header4/test.desc | 7 + .../function-comment-header5/main.cpp | 24 +++ .../function-comment-header5/test.desc | 10 ++ regression/cpp-linter/if-else1/main.cpp | 12 ++ regression/cpp-linter/if-else1/test.desc | 10 +- regression/cpp-linter/if-else2/main.cpp | 12 ++ regression/cpp-linter/if-else2/test.desc | 6 +- regression/cpp-linter/if-else3/main.cpp | 12 ++ regression/cpp-linter/if-else3/test.desc | 6 +- regression/cpp-linter/if-else4/main.cpp | 12 ++ regression/cpp-linter/if-else4/test.desc | 5 +- regression/cpp-linter/if-else5/main.cpp | 12 ++ regression/cpp-linter/if-else6/main.cpp | 12 ++ .../cpp-linter/multi-line-comment/main.cpp | 14 +- .../cpp-linter/multi-line-string1/main.cpp | 12 ++ .../cpp-linter/multi-line-string2/main.cpp | 12 ++ .../cpp-linter/operator-spacing1/main.cpp | 12 ++ .../cpp-linter/operator-spacing1/test.desc | 2 +- .../cpp-linter/operator-spacing2/main.cpp | 12 ++ .../cpp-linter/operator-spacing2/test.desc | 2 +- .../cpp-linter/operator-spacing3/main.cpp | 12 ++ .../cpp-linter/operator-spacing3/test.desc | 8 +- .../cpp-linter/prefix-increment/main.cpp | 12 ++ regression/cpp-linter/template-types/main.cpp | 12 ++ .../cpp-linter/template-types/test.desc | 2 +- scripts/cpplint.py | 144 ++++++++++++++++++ 38 files changed, 534 insertions(+), 25 deletions(-) create mode 100644 regression/cpp-linter/function-comment-header1/main.cpp create mode 100644 regression/cpp-linter/function-comment-header1/test.desc create mode 100644 regression/cpp-linter/function-comment-header2/main.cpp create mode 100644 regression/cpp-linter/function-comment-header2/test.desc create mode 100644 regression/cpp-linter/function-comment-header3/main.cpp create mode 100644 regression/cpp-linter/function-comment-header3/test.desc create mode 100644 regression/cpp-linter/function-comment-header4/main.cpp create mode 100644 regression/cpp-linter/function-comment-header4/test.desc create mode 100644 regression/cpp-linter/function-comment-header5/main.cpp create mode 100644 regression/cpp-linter/function-comment-header5/test.desc diff --git a/regression/cpp-linter/catch-cast/main.cpp b/regression/cpp-linter/catch-cast/main.cpp index 204d83f935c..1a356bc6626 100644 --- a/regression/cpp-linter/catch-cast/main.cpp +++ b/regression/cpp-linter/catch-cast/main.cpp @@ -6,7 +6,19 @@ Author: Thomas Kiley, thomas@diffblue.com \*******************************************************************/ -static fun() +/*******************************************************************\ + +Function: fun + + Inputs: + + Outputs: + + Purpose: + +\*******************************************************************/ + +static void fun() { catch(int) } diff --git a/regression/cpp-linter/do-while1/main.cpp b/regression/cpp-linter/do-while1/main.cpp index dfc0e2442ed..a6902b37060 100644 --- a/regression/cpp-linter/do-while1/main.cpp +++ b/regression/cpp-linter/do-while1/main.cpp @@ -6,6 +6,18 @@ Author: Thomas Kiley, thomas@diffblue.com \*******************************************************************/ +/*******************************************************************\ + +Function: fun + + Inputs: + + Outputs: + + Purpose: + +\*******************************************************************/ + static void fun() { // This should not produce a warning diff --git a/regression/cpp-linter/do-while1/test.desc b/regression/cpp-linter/do-while1/test.desc index 126809f8684..95e46922ded 100644 --- a/regression/cpp-linter/do-while1/test.desc +++ b/regression/cpp-linter/do-while1/test.desc @@ -1,8 +1,8 @@ CORE main.cpp -^main\.cpp:19: Empty loop bodies should use {} or continue \[whitespace/empty_loop_body\] \[5\]$ -^main\.cpp:26: Empty loop bodies should use {} or continue \[whitespace/empty_loop_body\] \[5\]$ +^main\.cpp:31: Empty loop bodies should use {} or continue \[whitespace/empty_loop_body\] \[5\]$ +^main\.cpp:38: Empty loop bodies should use {} or continue \[whitespace/empty_loop_body\] \[5\]$ ^Total errors found: 2$ ^SIGNAL=0$ -- diff --git a/regression/cpp-linter/do-while2/main.cpp b/regression/cpp-linter/do-while2/main.cpp index a3bfe00defa..9e2e1b0c352 100644 --- a/regression/cpp-linter/do-while2/main.cpp +++ b/regression/cpp-linter/do-while2/main.cpp @@ -6,6 +6,18 @@ Author: Thomas Kiley, thomas@diffblue.com \*******************************************************************/ +/*******************************************************************\ + +Function: fun + + Inputs: + + Outputs: + + Purpose: + +\*******************************************************************/ + static void fun() { do diff --git a/regression/cpp-linter/do-while2/test.desc b/regression/cpp-linter/do-while2/test.desc index 1fe4e46ab9b..97ef6886722 100644 --- a/regression/cpp-linter/do-while2/test.desc +++ b/regression/cpp-linter/do-while2/test.desc @@ -1,7 +1,7 @@ CORE main.cpp -^main\.cpp:14: while statement of do...while loop should be on a separate line to the closing brace \[readability/braces\] \[4\] +^main\.cpp:26: while statement of do...while loop should be on a separate line to the closing brace \[readability/braces\] \[4\] ^Total errors found: 1$ ^SIGNAL=0$ -- diff --git a/regression/cpp-linter/function-comment-header1/main.cpp b/regression/cpp-linter/function-comment-header1/main.cpp new file mode 100644 index 00000000000..9e66affff48 --- /dev/null +++ b/regression/cpp-linter/function-comment-header1/main.cpp @@ -0,0 +1,30 @@ +/*******************************************************************\ + +Module: Lint Examples + +Author: Thomas Kiley, thomas@diffblue.com + +\*******************************************************************/ + +/*******************************************************************\ + +Function: fun + + Inputs: + + Outputs: + + Purpose: + +\*******************************************************************/ + +static void fun() +{ + do_something(); +} + +static void foo() +{ + // No function header + do_something(); +} diff --git a/regression/cpp-linter/function-comment-header1/test.desc b/regression/cpp-linter/function-comment-header1/test.desc new file mode 100644 index 00000000000..efaae0b7391 --- /dev/null +++ b/regression/cpp-linter/function-comment-header1/test.desc @@ -0,0 +1,8 @@ +CORE +main.cpp + +^main\.cpp:26: Could not find function header comment for foo \[readability/function_comment\] \[4\] +^Total errors found: 1$ + +^SIGNAL=0$ +-- diff --git a/regression/cpp-linter/function-comment-header2/main.cpp b/regression/cpp-linter/function-comment-header2/main.cpp new file mode 100644 index 00000000000..efb7b9433b4 --- /dev/null +++ b/regression/cpp-linter/function-comment-header2/main.cpp @@ -0,0 +1,18 @@ +/*******************************************************************\ + +Module: Lint Examples + +Author: Thomas Kiley, thomas@diffblue.com + +\*******************************************************************/ + +/*******************************************************************\ + +Function: fun + +\*******************************************************************/ + +static void fun() +{ + do_something(); +} diff --git a/regression/cpp-linter/function-comment-header2/test.desc b/regression/cpp-linter/function-comment-header2/test.desc new file mode 100644 index 00000000000..639c28c6fe3 --- /dev/null +++ b/regression/cpp-linter/function-comment-header2/test.desc @@ -0,0 +1,9 @@ +CORE +main.cpp + +main\.cpp:15: Function header for fun missing Inputs: \[readability/function_comment\] \[4\] +main\.cpp:15: Function header for fun missing Outputs: \[readability/function_comment\] \[4\] +main\.cpp:15: Function header for fun missing Purpose: \[readability/function_comment\] \[4\] +^Total errors found: 3$ +^SIGNAL=0$ +-- diff --git a/regression/cpp-linter/function-comment-header3/main.cpp b/regression/cpp-linter/function-comment-header3/main.cpp new file mode 100644 index 00000000000..f2596186224 --- /dev/null +++ b/regression/cpp-linter/function-comment-header3/main.cpp @@ -0,0 +1,23 @@ +/*******************************************************************\ + +Module: Lint Examples + +Author: Thomas Kiley, thomas@diffblue.com + +\*******************************************************************/ + +/*******************************************************************\ + +Function: fun + + Inputs: + + Outputs: + + Purpose: + +\*******************************************************************/ +static void fun() +{ + do_something(); +} diff --git a/regression/cpp-linter/function-comment-header3/test.desc b/regression/cpp-linter/function-comment-header3/test.desc new file mode 100644 index 00000000000..9be52904021 --- /dev/null +++ b/regression/cpp-linter/function-comment-header3/test.desc @@ -0,0 +1,7 @@ +CORE +main.cpp + +^main\.cpp:20: Insert an empty line between function header comment and the function fun \[readability/function_comment\] \[4\]$ +^Total errors found: 1$ +^SIGNAL=0$ +-- diff --git a/regression/cpp-linter/function-comment-header4/main.cpp b/regression/cpp-linter/function-comment-header4/main.cpp new file mode 100644 index 00000000000..73b5b477d6c --- /dev/null +++ b/regression/cpp-linter/function-comment-header4/main.cpp @@ -0,0 +1,24 @@ +/*******************************************************************\ + +Module: Lint Examples + +Author: Thomas Kiley, thomas@diffblue.com + +\*******************************************************************/ + +/*******************************************************************\ + +Function: wrong_name + + Inputs: + + Outputs: + + Purpose: + +\*******************************************************************/ + +static void fun() +{ + do_something(); +} diff --git a/regression/cpp-linter/function-comment-header4/test.desc b/regression/cpp-linter/function-comment-header4/test.desc new file mode 100644 index 00000000000..0766623cc21 --- /dev/null +++ b/regression/cpp-linter/function-comment-header4/test.desc @@ -0,0 +1,7 @@ +CORE +main.cpp + +^main\.cpp:11: Function: name in the comment doesn.t match the function name \[readability/function_comment\] \[4\] +^Total errors found: 1$ +^SIGNAL=0$ +-- diff --git a/regression/cpp-linter/function-comment-header5/main.cpp b/regression/cpp-linter/function-comment-header5/main.cpp new file mode 100644 index 00000000000..74f507aded0 --- /dev/null +++ b/regression/cpp-linter/function-comment-header5/main.cpp @@ -0,0 +1,24 @@ +/*******************************************************************\ + +Module: Lint Examples + +Author: Thomas Kiley, thomas@diffblue.com + +\*******************************************************************/ + +/*******************************************************************\ + + Function: fun + + Inputs: + +Outputs: + + Purpose: + +\*******************************************************************/ + +static void fun() +{ + do_something(); +} diff --git a/regression/cpp-linter/function-comment-header5/test.desc b/regression/cpp-linter/function-comment-header5/test.desc new file mode 100644 index 00000000000..dd780176481 --- /dev/null +++ b/regression/cpp-linter/function-comment-header5/test.desc @@ -0,0 +1,10 @@ +CORE +main.cpp + +^main\.cpp:17: Purpose: should be indented one space in \[readability/function_comment\] \[4\]$ +^main\.cpp:15: Outputs: should be indented one space in \[readability/function_comment\] \[4\]$ +^main\.cpp:13: Inputs: should be indented one space in \[readability/function_comment\] \[4\]$ +^main\.cpp:11: Function: should not be indented in the function header \[readability/function_comment\] \[4\]$ +^Total errors found: 4$ +^SIGNAL=0$ +-- diff --git a/regression/cpp-linter/if-else1/main.cpp b/regression/cpp-linter/if-else1/main.cpp index 43d24bc47cd..f2f14f83eaf 100644 --- a/regression/cpp-linter/if-else1/main.cpp +++ b/regression/cpp-linter/if-else1/main.cpp @@ -6,6 +6,18 @@ Author: Thomas Kiley, thomas@diffblue.com \*******************************************************************/ +/*******************************************************************\ + +Function: fun + + Inputs: + + Outputs: + + Purpose: + +\*******************************************************************/ + static void fun() { bool condition=false; diff --git a/regression/cpp-linter/if-else1/test.desc b/regression/cpp-linter/if-else1/test.desc index a32899d0df0..c78c36bdc30 100644 --- a/regression/cpp-linter/if-else1/test.desc +++ b/regression/cpp-linter/if-else1/test.desc @@ -1,11 +1,11 @@ CORE main.cpp -^main\.cpp:12: Put braces on a separate next line \[whitespace/braces\] \[5\]$ -^main\.cpp:14: Put braces on a separate next line \[whitespace/braces\] \[5\]$ -^main\.cpp:14: Else if should be on a new line after closing brace \[readability/braces\] \[5\]$ -^main\.cpp:16: Put braces on a separate next line \[whitespace/braces\] \[5\]$ -^main\.cpp:16: Else should be on a new line after closing brace \[readability/braces\] \[5\]$ +^main\.cpp:24: Put braces on a separate next line \[whitespace/braces\] \[5\]$ +^main\.cpp:26: Put braces on a separate next line \[whitespace/braces\] \[5\]$ +^main\.cpp:26: Else if should be on a new line after closing brace \[readability/braces\] \[5\]$ +^main\.cpp:28: Put braces on a separate next line \[whitespace/braces\] \[5\]$ +^main\.cpp:28: Else should be on a new line after closing brace \[readability/braces\] \[5\]$ ^Total errors found: 5$ ^SIGNAL=0$ -- diff --git a/regression/cpp-linter/if-else2/main.cpp b/regression/cpp-linter/if-else2/main.cpp index c4c2d84b38c..0cf973f0e1c 100644 --- a/regression/cpp-linter/if-else2/main.cpp +++ b/regression/cpp-linter/if-else2/main.cpp @@ -6,6 +6,18 @@ Author: Thomas Kiley, thomas@diffblue.com \*******************************************************************/ +/*******************************************************************\ + +Function: fun + + Inputs: + + Outputs: + + Purpose: + +\*******************************************************************/ + static void fun() { if(condition) { diff --git a/regression/cpp-linter/if-else2/test.desc b/regression/cpp-linter/if-else2/test.desc index ef4d2be01e9..b60b15b039d 100644 --- a/regression/cpp-linter/if-else2/test.desc +++ b/regression/cpp-linter/if-else2/test.desc @@ -1,9 +1,9 @@ CORE main.cpp -^main\.cpp:11: Put braces on a separate next line \[whitespace/braces\] \[5\]$ -^main\.cpp:14: Put braces on a separate next line \[whitespace/braces\] \[5\]$ -^main\.cpp:17: Put braces on a separate next line \[whitespace/braces\] \[5\]$ +^main\.cpp:23: Put braces on a separate next line \[whitespace/braces\] \[5\]$ +^main\.cpp:26: Put braces on a separate next line \[whitespace/braces\] \[5\]$ +^main\.cpp:29: Put braces on a separate next line \[whitespace/braces\] \[5\]$ ^Total errors found: 3$ ^SIGNAL=0$ -- diff --git a/regression/cpp-linter/if-else3/main.cpp b/regression/cpp-linter/if-else3/main.cpp index 035be757c17..b3fb3a974ee 100644 --- a/regression/cpp-linter/if-else3/main.cpp +++ b/regression/cpp-linter/if-else3/main.cpp @@ -6,6 +6,18 @@ Author: Thomas Kiley, thomas@diffblue.com \*******************************************************************/ +/*******************************************************************\ + +Function: fun + + Inputs: + + Outputs: + + Purpose: + +\*******************************************************************/ + static void fun() { bool condition=false; diff --git a/regression/cpp-linter/if-else3/test.desc b/regression/cpp-linter/if-else3/test.desc index 0a0073495ad..0199293038f 100644 --- a/regression/cpp-linter/if-else3/test.desc +++ b/regression/cpp-linter/if-else3/test.desc @@ -1,9 +1,9 @@ CORE main.cpp -^main\.cpp:12: Statement after an if should be on a new line \[readability/braces\] \[5\]$ -^main\.cpp:13: Statement after else if should be on a new line \[readability/braces\] \[5\]$ -^main\.cpp:14: Statement after else should be on a new line \[readability/braces\] \[5\]$ +^main\.cpp:24: Statement after an if should be on a new line \[readability/braces\] \[5\]$ +^main\.cpp:25: Statement after else if should be on a new line \[readability/braces\] \[5\]$ +^main\.cpp:26: Statement after else should be on a new line \[readability/braces\] \[5\]$ ^Total errors found: 3$ ^SIGNAL=0$ -- diff --git a/regression/cpp-linter/if-else4/main.cpp b/regression/cpp-linter/if-else4/main.cpp index 0d599dd0e36..759c147276b 100644 --- a/regression/cpp-linter/if-else4/main.cpp +++ b/regression/cpp-linter/if-else4/main.cpp @@ -6,6 +6,18 @@ Author: Thomas Kiley, thomas@diffblue.com \*******************************************************************/ +/*******************************************************************\ + +Function: fun + + Inputs: + + Outputs: + + Purpose: + +\*******************************************************************/ + static void fun() { bool condition=false; diff --git a/regression/cpp-linter/if-else4/test.desc b/regression/cpp-linter/if-else4/test.desc index 8aac427632a..0199293038f 100644 --- a/regression/cpp-linter/if-else4/test.desc +++ b/regression/cpp-linter/if-else4/test.desc @@ -1,8 +1,9 @@ CORE main.cpp -^main\.cpp:12: Statement after an if should be on a new line \[readability/braces\] \[5\]$ -^main\.cpp:13: Statement after else if should be on a new line \[readability/braces\] \[5\]$ +^main\.cpp:24: Statement after an if should be on a new line \[readability/braces\] \[5\]$ +^main\.cpp:25: Statement after else if should be on a new line \[readability/braces\] \[5\]$ +^main\.cpp:26: Statement after else should be on a new line \[readability/braces\] \[5\]$ ^Total errors found: 3$ ^SIGNAL=0$ -- diff --git a/regression/cpp-linter/if-else5/main.cpp b/regression/cpp-linter/if-else5/main.cpp index a635b257c38..f5384511a07 100644 --- a/regression/cpp-linter/if-else5/main.cpp +++ b/regression/cpp-linter/if-else5/main.cpp @@ -6,6 +6,18 @@ Author: Thomas Kiley, thomas@diffblue.com \*******************************************************************/ +/*******************************************************************\ + +Function: fun + + Inputs: + + Outputs: + + Purpose: + +\*******************************************************************/ + static void fun() { bool condition=false; diff --git a/regression/cpp-linter/if-else6/main.cpp b/regression/cpp-linter/if-else6/main.cpp index ae62578a440..cf44895e9df 100644 --- a/regression/cpp-linter/if-else6/main.cpp +++ b/regression/cpp-linter/if-else6/main.cpp @@ -6,6 +6,18 @@ Author: Thomas Kiley, thomas@diffblue.com \*******************************************************************/ +/*******************************************************************\ + +Function: fun + + Inputs: + + Outputs: + + Purpose: + +\*******************************************************************/ + static void fun() { bool condition=false; diff --git a/regression/cpp-linter/multi-line-comment/main.cpp b/regression/cpp-linter/multi-line-comment/main.cpp index 1e708f42e3d..9e53421a1de 100644 --- a/regression/cpp-linter/multi-line-comment/main.cpp +++ b/regression/cpp-linter/multi-line-comment/main.cpp @@ -6,7 +6,19 @@ Author: Thomas Kiley, thomas@diffblue.com \*******************************************************************/ -static fun() +/*******************************************************************\ + +Function: fun + + Inputs: + + Outputs: + + Purpose: + +\*******************************************************************/ + +static void fun() { // A multi line comment (that includes an opening // bracket and a closing one on the next line) diff --git a/regression/cpp-linter/multi-line-string1/main.cpp b/regression/cpp-linter/multi-line-string1/main.cpp index 436fe52a8db..39cfb1d0abf 100644 --- a/regression/cpp-linter/multi-line-string1/main.cpp +++ b/regression/cpp-linter/multi-line-string1/main.cpp @@ -6,6 +6,18 @@ Author: Thomas Kiley, thomas@diffblue.com \*******************************************************************/ +/*******************************************************************\ + +Function: fun + + Inputs: + + Outputs: + + Purpose: + +\*******************************************************************/ + static void fun() { auto fn_prettyname=id2string(arg0.get(ID_C_class)) diff --git a/regression/cpp-linter/multi-line-string2/main.cpp b/regression/cpp-linter/multi-line-string2/main.cpp index 73f0fa52adc..a380fbd1a37 100644 --- a/regression/cpp-linter/multi-line-string2/main.cpp +++ b/regression/cpp-linter/multi-line-string2/main.cpp @@ -6,6 +6,18 @@ Author: Thomas Kiley, thomas@diffblue.com \*******************************************************************/ +/*******************************************************************\ + +Function: fun + + Inputs: + + Outputs: + + Purpose: + +\*******************************************************************/ + static void fun() { status() << "Adding CPROVER library (" diff --git a/regression/cpp-linter/operator-spacing1/main.cpp b/regression/cpp-linter/operator-spacing1/main.cpp index eebfb7cb5b3..298c4665283 100644 --- a/regression/cpp-linter/operator-spacing1/main.cpp +++ b/regression/cpp-linter/operator-spacing1/main.cpp @@ -6,6 +6,18 @@ Author: Thomas Kiley, thomas@diffblue.com \*******************************************************************/ +/*******************************************************************\ + +Function: fun + + Inputs: + + Outputs: + + Purpose: + +\*******************************************************************/ + static void fun() { version_set = true; diff --git a/regression/cpp-linter/operator-spacing1/test.desc b/regression/cpp-linter/operator-spacing1/test.desc index 67b95af2d0b..c988ced295b 100644 --- a/regression/cpp-linter/operator-spacing1/test.desc +++ b/regression/cpp-linter/operator-spacing1/test.desc @@ -1,7 +1,7 @@ CORE main.cpp -^main\.cpp:11: Remove spaces around = \[whitespace/operators\] \[4\] +^main\.cpp:23: Remove spaces around = \[whitespace/operators\] \[4\] ^Total errors found: 1$ ^SIGNAL=0$ -- diff --git a/regression/cpp-linter/operator-spacing2/main.cpp b/regression/cpp-linter/operator-spacing2/main.cpp index c11967d0eb0..90db55a167e 100644 --- a/regression/cpp-linter/operator-spacing2/main.cpp +++ b/regression/cpp-linter/operator-spacing2/main.cpp @@ -6,6 +6,18 @@ Author: Thomas Kiley, thomas@diffblue.com \*******************************************************************/ +/*******************************************************************\ + +Function: fun + + Inputs: + + Outputs: + + Purpose: + +\*******************************************************************/ + static void fun() { status() << "Adding CPROVER library (" << eom; diff --git a/regression/cpp-linter/operator-spacing2/test.desc b/regression/cpp-linter/operator-spacing2/test.desc index 1f6b97abd43..b5e8b7a9102 100644 --- a/regression/cpp-linter/operator-spacing2/test.desc +++ b/regression/cpp-linter/operator-spacing2/test.desc @@ -1,7 +1,7 @@ CORE main.cpp -^main\.cpp:15: Missing spaces around << \[whitespace/operators\] \[3\] +^main\.cpp:27: Missing spaces around << \[whitespace/operators\] \[3\] ^Total errors found: 1$ ^SIGNAL=0$ -- diff --git a/regression/cpp-linter/operator-spacing3/main.cpp b/regression/cpp-linter/operator-spacing3/main.cpp index b39a0c63d3c..eba8bc9a8b2 100644 --- a/regression/cpp-linter/operator-spacing3/main.cpp +++ b/regression/cpp-linter/operator-spacing3/main.cpp @@ -6,6 +6,18 @@ Author: Thomas Kiley, thomas@diffblue.com \*******************************************************************/ +/*******************************************************************\ + +Function: fun + + Inputs: + + Outputs: + + Purpose: + +\*******************************************************************/ + static void fun() { status() << "Adding CPROVER library (" << eom; diff --git a/regression/cpp-linter/operator-spacing3/test.desc b/regression/cpp-linter/operator-spacing3/test.desc index d5bb40b89d0..056369e4df6 100644 --- a/regression/cpp-linter/operator-spacing3/test.desc +++ b/regression/cpp-linter/operator-spacing3/test.desc @@ -1,10 +1,10 @@ KNOWNBUG main.cpp -^main\.cpp:15: Missing spaces around << \[whitespace/operators\] \[3\] -^main\.cpp:15: Missing spaces around << \[whitespace/operators\] \[3\] -^main\.cpp:17: Remove spaces around << \[whitespace/operators\] \[4\] -^main\.cpp:17: Remove spaces around << \[whitespace/operators\] \[4\] +^main\.cpp:27: Missing spaces around << \[whitespace/operators\] \[3\] +^main\.cpp:27: Missing spaces around << \[whitespace/operators\] \[3\] +^main\.cpp:29: Remove spaces around << \[whitespace/operators\] \[4\] +^main\.cpp:29: Remove spaces around << \[whitespace/operators\] \[4\] ^Total errors found: 4$ ^SIGNAL=0$ -- diff --git a/regression/cpp-linter/prefix-increment/main.cpp b/regression/cpp-linter/prefix-increment/main.cpp index c63302d19f4..e805cebc6d2 100644 --- a/regression/cpp-linter/prefix-increment/main.cpp +++ b/regression/cpp-linter/prefix-increment/main.cpp @@ -6,6 +6,18 @@ Author: Thomas Kiley, thomas@diffblue.com \*******************************************************************/ +/*******************************************************************\ + +Function: fun + + Inputs: + + Outputs: + + Purpose: + +\*******************************************************************/ + static void fun() { for(int i=0; i<10; ++i) diff --git a/regression/cpp-linter/template-types/main.cpp b/regression/cpp-linter/template-types/main.cpp index a221aadb0f0..7ab1ad73d73 100644 --- a/regression/cpp-linter/template-types/main.cpp +++ b/regression/cpp-linter/template-types/main.cpp @@ -6,6 +6,18 @@ Author: Thomas Kiley, thomas@diffblue.com \*******************************************************************/ +/*******************************************************************\ + +Function: fun + + Inputs: + + Outputs: + + Purpose: + +\*******************************************************************/ + static void fun() { typedef std::vector number_listt; diff --git a/regression/cpp-linter/template-types/test.desc b/regression/cpp-linter/template-types/test.desc index b5d31ac3947..280b37b71b0 100644 --- a/regression/cpp-linter/template-types/test.desc +++ b/regression/cpp-linter/template-types/test.desc @@ -1,7 +1,7 @@ CORE main.cpp -^main\.cpp:17: Remove spaces around > \[whitespace/operators\] \[4\]$ +^main\.cpp:29: Remove spaces around > \[whitespace/operators\] \[4\]$ ^Total errors found: 1$ ^SIGNAL=0$ -- diff --git a/scripts/cpplint.py b/scripts/cpplint.py index 8e67b9510f6..b0a1dc2c3da 100644 --- a/scripts/cpplint.py +++ b/scripts/cpplint.py @@ -224,6 +224,7 @@ 'readability/strings', 'readability/todo', 'readability/utf8', + 'readability/function_comment' 'runtime/arrays', 'runtime/casting', 'runtime/explicit', @@ -3080,6 +3081,147 @@ def CheckForNamespaceIndentation(filename, nesting_state, clean_lines, line, CheckItemIndentationInNamespace(filename, clean_lines.elided, line, error) +def CheckForFunctionCommentHeaders(filename, raw_lines, error): + """ Check all the lines for functions without function comment headers + + Using the raw original lines (before multi-line comments are removed) + find lines that contain functions and checks they have a function comment header + + Args: + filename - The name of the file being checked + raw_lines - The original, with comments lines + error - the function to report errors with + """ + linenum = 0 + function_state = _FunctionState() + for line in raw_lines: + joined_line = '' + starting_func = False + regexp = r'(\w(\w|::|\*|\&|\s)*)\(' # decls * & space::name( ... + match_result = Match(regexp, line) + if match_result: + # If the name is all caps and underscores, figure it's a macro and + # ignore it, unless it's TEST or TEST_F. + function_name = match_result.group(1).split()[-1] + if function_name == 'TEST' or function_name == 'TEST_F' or ( + not Match(r'[A-Z_]+$', function_name)): + starting_func = True + + if starting_func: + body_found = False + for start_linenum in xrange(linenum, len(raw_lines)): + start_line = raw_lines[start_linenum] + joined_line += ' ' + start_line.lstrip() + if Search(r'(;|})', start_line): # Declarations and trivial functions + body_found = True + break # ... ignore + elif Search(r'{', start_line): + body_found = True + function = Search(r'((\w|:)*)\(', line).group(1) + if Match(r'TEST', function): # Handle TEST... macros + parameter_regexp = Search(r'(\(.*\))', joined_line) + if parameter_regexp: # Ignore bad syntax + function += parameter_regexp.group(1) + else: + function += '()' + function_state.Begin(function) + break + if not body_found: + # No body for the function (or evidence of a non-function) was found. + error(filename, linenum, 'readability/fn_size', 5, + 'Lint failed to find start of function body.') + else: + CheckForFunctionCommentHeader(filename, raw_lines, linenum, function_name, error) + linenum += 1 + +def CheckForFunctionCommentHeader(filename, raw_lines, linenum, function_name, error): + """ Check each function has a comment header + + Rules for function comment header: + 1. Header demarked by /*{67}\ at the top + 2. Header demarked by \*{67}/ at the bottom + 3. Contains Function: classname::function_name + 4. Contains Inputs: + 5. Contains Outputs: + 6. Contains Purpose: + 7. new line between function and bottom of comment + + Args: + filename - the name of the file + raw_lines - all the unmodified lines (including multi-line comments) + linenum - the line number of the line to check + function_name - the name of the function that was found + error - function to report errors with + + """ + + header_top_regex = r'^/\*{67}\\$' + header_bottom_regex = r'^\\\*{67}/$' + function_name_regex = r'Function: (\w+::)?' + function_name+'$' + + found_empty_space = raw_lines[linenum-1] == "" + + header_start = linenum - 2 if found_empty_space else linenum - 1 + found_header_bottom = Match(header_bottom_regex, raw_lines[header_start]) + + if not found_header_bottom: + error(filename, linenum, 'readability/function_comment', 4, + 'Could not find function header comment for ' + function_name) + return + + found_header_top = False + + found_function_name = False + found_inputs = False + found_outputs = False + found_purpose = False + + for i in xrange(header_start - 1, 0, -1): + if(Match(header_top_regex, raw_lines[i])): + found_header_top = True + break + + if(Search(r'Inputs:', raw_lines[i])): + found_inputs = True + if(not Match(r'^ Inputs:', raw_lines[i])): + error(filename, i, 'readability/function_comment', 4, + 'Inputs: should be indented one space in') + elif(Search(r'Outputs:', raw_lines[i])): + found_outputs = True + if(not Match(r'^ Outputs:', raw_lines[i])): + error(filename, i, 'readability/function_comment', 4, + 'Outputs: should be indented one space in') + elif(Search(r'Purpose:', raw_lines[i])): + found_purpose = True + if(not Match(r'^ Purpose:', raw_lines[i])): + error(filename, i, 'readability/function_comment', 4, + 'Purpose: should be indented one space in') + elif(Search(r'Function:', raw_lines[i])): + found_function_name = True + if(not Search(function_name_regex, raw_lines[i])): + error(filename, i, 'readability/function_comment', 4, + 'Function: name in the comment doesn\'t match the function name') + if(not Match(r'^Function:', raw_lines[i])): + error(filename, i, 'readability/function_comment', 4, + 'Function: should not be indented in the function header') + + if found_header_top: + if not found_inputs: + error(filename, linenum, 'readability/function_comment', 4, + 'Function header for ' + function_name + ' missing Inputs:') + if not found_outputs: + error(filename, linenum, 'readability/function_comment', 4, + 'Function header for ' + function_name + ' missing Outputs:') + if not found_purpose: + error(filename, linenum, 'readability/function_comment', 4, + 'Function header for ' + function_name + ' missing Purpose:') + else: + error(filename, linenum, 'readability/function_comment', 4, + 'Could not find top of function header comment for ' + function_name) + + if not found_empty_space: + error(filename, linenum, 'readability/function_comment', 4, + 'Insert an empty line between function header comment and the function ' + function_name) def CheckForFunctionLengths(filename, clean_lines, linenum, function_state, error): @@ -3142,6 +3284,7 @@ def CheckForFunctionLengths(filename, clean_lines, linenum, # No body for the function (or evidence of a non-function) was found. error(filename, linenum, 'readability/fn_size', 5, 'Lint failed to find start of function body.') + elif Match(r'^\}\s*$', line): # function end function_state.Check(error, filename, linenum) function_state.End() @@ -6027,6 +6170,7 @@ def ProcessFileData(filename, file_extension, lines, error, ResetNolintSuppressions() CheckForCopyright(filename, lines, error) + CheckForFunctionCommentHeaders(filename, lines, error) ProcessGlobalSuppresions(lines) RemoveMultiLineComments(filename, lines, error) clean_lines = CleansedLines(lines) From 6c7fe16e36c31b238683676ec98a89a548c9a9be Mon Sep 17 00:00:00 2001 From: thk123 Date: Wed, 21 Dec 2016 14:23:36 +0000 Subject: [PATCH 23/30] Addded description of the function comment header to coding standard --- CODING_STANDARD | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/CODING_STANDARD b/CODING_STANDARD index 07d061c3130..725338e3fd4 100644 --- a/CODING_STANDARD +++ b/CODING_STANDARD @@ -31,9 +31,31 @@ Comments: - Do not use /* */ except for file and function comment blocks - Each source and header file must start with a comment block stating the Module name and Author [will be changed when we roll out doxygen] -- Each function in the source file (not the header) is preceded - by a comment block stating Name, Inputs, Outputs and Purpose [will be changed - when we roll out doxygen] +- Each function in the source file (not the header) is preceded + by a function comment header consisting of a comment block stating + Name, Inputs, Outputs and Purpose [will be changed when we roll + out doxygen] + - It should look like this: + ``` + /*******************************************************************\ + + Function: class_namet::function_name + + Inputs: + arg_name - Description of its purpose + long_arg_name - Descriptions should be indented + to match the first line of the + description + + Outputs: A description of what the function returns + + Purpose: A description of what the function does. + Again, indentation with the line above + + \*******************************************************************/ + ``` +- An empty line should appear between the bottom of the function comment header + and the function. - Put comments on a separate line - Use comments to explain the non-obvious - Use #if 0 for commenting out source code From 141841082852049accd9448f970839bfd9ca3d80 Mon Sep 17 00:00:00 2001 From: thk123 Date: Wed, 21 Dec 2016 14:55:40 +0000 Subject: [PATCH 24/30] Reduced restrictions on the function comment header Removed specifying whitespace of the contents of the comment header. It is fairly inconsistent across the code base and we are hopefully moving to doxygen style comments in the near future so no point wasting a lot of time fixing up these comments. --- scripts/cpplint.py | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/scripts/cpplint.py b/scripts/cpplint.py index b0a1dc2c3da..43bb6836ba8 100644 --- a/scripts/cpplint.py +++ b/scripts/cpplint.py @@ -3183,27 +3183,15 @@ def CheckForFunctionCommentHeader(filename, raw_lines, linenum, function_name, e if(Search(r'Inputs:', raw_lines[i])): found_inputs = True - if(not Match(r'^ Inputs:', raw_lines[i])): - error(filename, i, 'readability/function_comment', 4, - 'Inputs: should be indented one space in') elif(Search(r'Outputs:', raw_lines[i])): found_outputs = True - if(not Match(r'^ Outputs:', raw_lines[i])): - error(filename, i, 'readability/function_comment', 4, - 'Outputs: should be indented one space in') elif(Search(r'Purpose:', raw_lines[i])): found_purpose = True - if(not Match(r'^ Purpose:', raw_lines[i])): - error(filename, i, 'readability/function_comment', 4, - 'Purpose: should be indented one space in') elif(Search(r'Function:', raw_lines[i])): found_function_name = True if(not Search(function_name_regex, raw_lines[i])): error(filename, i, 'readability/function_comment', 4, 'Function: name in the comment doesn\'t match the function name') - if(not Match(r'^Function:', raw_lines[i])): - error(filename, i, 'readability/function_comment', 4, - 'Function: should not be indented in the function header') if found_header_top: if not found_inputs: From 0b35b4f32189a189b42b5b59fb99fea58abbec54 Mon Sep 17 00:00:00 2001 From: thk123 Date: Wed, 21 Dec 2016 15:21:57 +0000 Subject: [PATCH 25/30] Extended coding guidelines to cover auto and forward declarations --- CODING_STANDARD | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CODING_STANDARD b/CODING_STANDARD index 725338e3fd4..38974907072 100644 --- a/CODING_STANDARD +++ b/CODING_STANDARD @@ -75,6 +75,8 @@ Naming: Header files: - Avoid unnecessary #includes, especially in header files +- Prefer forward declaration to includes, but forward declare at the top + of the header file rather than in line - Guard headers with #ifndef CPROVER_DIRECTORIES_FILE_H, etc C++ features: @@ -109,6 +111,9 @@ C++ features: - We allow to use 3rd-party libraries directly. No wrapper matching the coding rules is required. Allowed libraries are: STL. +- Use the auto keyword if and only if one of the following + - The type is explictly repeated on the RHS (e.g. a constructor call) + - Adding the type will increase confusion (e.g. iterators, function pointers) Architecture-specific code: - Avoid if possible. From 9a6ea45f1dae2ea1fed6f0cb0eb24ed5d681731f Mon Sep 17 00:00:00 2001 From: thk123 Date: Wed, 21 Dec 2016 16:12:49 +0000 Subject: [PATCH 26/30] Added more explicit rules for breaking long lines --- CODING_STANDARD | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CODING_STANDARD b/CODING_STANDARD index 38974907072..3466cd05c99 100644 --- a/CODING_STANDARD +++ b/CODING_STANDARD @@ -3,6 +3,14 @@ Here a few minimalistic coding rules for the CPROVER source tree. Whitespaces: - Use 2 spaces indent, no tabs. - No lines wider than 80 chars. + - When line is wider, do the following: + - Subsequent lines should be indented two more than the initial line + - Break after = if it is part of an assignment + - For chained calls, prefer immediately before . + - For other operators (e.g. &&, +) prefer immediately after the operator + - For brackets, break after the bracket + - In the case of function calls, put each argument on a separate line if + they do not fit after one line break - If a method is bigger than 50 lines, break it into parts. - Put matching { } into the same column. - No spaces around operators (=, +, ==, ...) From fa53b71ba5c0f6a64c8fc76a45b3d8db227331f5 Mon Sep 17 00:00:00 2001 From: thk123 Date: Thu, 22 Dec 2016 10:58:14 +0000 Subject: [PATCH 27/30] Removing tests for indentation --- .../function-comment-header5/main.cpp | 24 ------------------- .../function-comment-header5/test.desc | 10 -------- 2 files changed, 34 deletions(-) delete mode 100644 regression/cpp-linter/function-comment-header5/main.cpp delete mode 100644 regression/cpp-linter/function-comment-header5/test.desc diff --git a/regression/cpp-linter/function-comment-header5/main.cpp b/regression/cpp-linter/function-comment-header5/main.cpp deleted file mode 100644 index 74f507aded0..00000000000 --- a/regression/cpp-linter/function-comment-header5/main.cpp +++ /dev/null @@ -1,24 +0,0 @@ -/*******************************************************************\ - -Module: Lint Examples - -Author: Thomas Kiley, thomas@diffblue.com - -\*******************************************************************/ - -/*******************************************************************\ - - Function: fun - - Inputs: - -Outputs: - - Purpose: - -\*******************************************************************/ - -static void fun() -{ - do_something(); -} diff --git a/regression/cpp-linter/function-comment-header5/test.desc b/regression/cpp-linter/function-comment-header5/test.desc deleted file mode 100644 index dd780176481..00000000000 --- a/regression/cpp-linter/function-comment-header5/test.desc +++ /dev/null @@ -1,10 +0,0 @@ -CORE -main.cpp - -^main\.cpp:17: Purpose: should be indented one space in \[readability/function_comment\] \[4\]$ -^main\.cpp:15: Outputs: should be indented one space in \[readability/function_comment\] \[4\]$ -^main\.cpp:13: Inputs: should be indented one space in \[readability/function_comment\] \[4\]$ -^main\.cpp:11: Function: should not be indented in the function header \[readability/function_comment\] \[4\]$ -^Total errors found: 4$ -^SIGNAL=0$ --- From 7638907c04d41f9788c91ca6f7f5a9cbed97a4f2 Mon Sep 17 00:00:00 2001 From: thk123 Date: Thu, 22 Dec 2016 11:01:13 +0000 Subject: [PATCH 28/30] Don't include * or & in function name When checking for function comments we don't want to include the * or & in the name. Instead we allow after the space of the return type an optional * or & before accepting the function name. --- .../function-comment-header5/main.cpp | 24 +++++++++++++++++++ .../function-comment-header5/test.desc | 7 ++++++ scripts/cpplint.py | 6 +++-- 3 files changed, 35 insertions(+), 2 deletions(-) create mode 100644 regression/cpp-linter/function-comment-header5/main.cpp create mode 100644 regression/cpp-linter/function-comment-header5/test.desc diff --git a/regression/cpp-linter/function-comment-header5/main.cpp b/regression/cpp-linter/function-comment-header5/main.cpp new file mode 100644 index 00000000000..3933fdaa3f8 --- /dev/null +++ b/regression/cpp-linter/function-comment-header5/main.cpp @@ -0,0 +1,24 @@ +/*******************************************************************\ + +Module: Lint Examples + +Author: Thomas Kiley, thomas@diffblue.com + +\*******************************************************************/ + +/*******************************************************************\ + +Function: fun + + Inputs: + + Outputs: + + Purpose: + +\*******************************************************************/ + +static int *fun() +{ + do_something(); +} diff --git a/regression/cpp-linter/function-comment-header5/test.desc b/regression/cpp-linter/function-comment-header5/test.desc new file mode 100644 index 00000000000..12418d892f4 --- /dev/null +++ b/regression/cpp-linter/function-comment-header5/test.desc @@ -0,0 +1,7 @@ +CORE +main.cpp + +^Total errors found: 0$ +^EXIT=0$ +^SIGNAL=0$ +-- diff --git a/scripts/cpplint.py b/scripts/cpplint.py index 43bb6836ba8..7ad7e462729 100644 --- a/scripts/cpplint.py +++ b/scripts/cpplint.py @@ -3097,12 +3097,14 @@ def CheckForFunctionCommentHeaders(filename, raw_lines, error): for line in raw_lines: joined_line = '' starting_func = False - regexp = r'(\w(\w|::|\*|\&|\s)*)\(' # decls * & space::name( ... + # Look for declaration function_name( but allowing for *, & being attached to the function name + # but not being considered part of it + regexp = r'\w(\w|::|\s|\*|\&)* (\*|\&)?(?P\w(\w|::)*)\(' match_result = Match(regexp, line) if match_result: # If the name is all caps and underscores, figure it's a macro and # ignore it, unless it's TEST or TEST_F. - function_name = match_result.group(1).split()[-1] + function_name = match_result.group('fnc_name') if function_name == 'TEST' or function_name == 'TEST_F' or ( not Match(r'[A-Z_]+$', function_name)): starting_func = True From 7374c77b008ed4bd80b47c705ed684858b3bbdc9 Mon Sep 17 00:00:00 2001 From: thk123 Date: Thu, 22 Dec 2016 16:27:12 +0000 Subject: [PATCH 29/30] Handle operator functions when looking for function comment headers Since operator functions can have non-word characters as part of their name (specifically, can have an opening bracket) we deal with these seperately in the explicit case where the functions name is operator. Since the function name can now contain non-word characters (e.g. an opening bracket) we must escape it before using it in the regex. --- .../function-comment-header6/main.cpp | 24 +++++++++++++++++++ .../function-comment-header6/test.desc | 7 ++++++ scripts/cpplint.py | 15 +++++++++--- 3 files changed, 43 insertions(+), 3 deletions(-) create mode 100644 regression/cpp-linter/function-comment-header6/main.cpp create mode 100644 regression/cpp-linter/function-comment-header6/test.desc diff --git a/regression/cpp-linter/function-comment-header6/main.cpp b/regression/cpp-linter/function-comment-header6/main.cpp new file mode 100644 index 00000000000..fdaf7938525 --- /dev/null +++ b/regression/cpp-linter/function-comment-header6/main.cpp @@ -0,0 +1,24 @@ +/*******************************************************************\ + +Module: Lint Examples + +Author: Thomas Kiley, thomas@diffblue.com + +\*******************************************************************/ + +/*******************************************************************\ + +Function: operator() + + Inputs: + + Outputs: + + Purpose: + +\*******************************************************************/ + +static void operator()() +{ + do_something(); +} diff --git a/regression/cpp-linter/function-comment-header6/test.desc b/regression/cpp-linter/function-comment-header6/test.desc new file mode 100644 index 00000000000..12418d892f4 --- /dev/null +++ b/regression/cpp-linter/function-comment-header6/test.desc @@ -0,0 +1,7 @@ +CORE +main.cpp + +^Total errors found: 0$ +^EXIT=0$ +^SIGNAL=0$ +-- diff --git a/scripts/cpplint.py b/scripts/cpplint.py index 7ad7e462729..39332ab3790 100644 --- a/scripts/cpplint.py +++ b/scripts/cpplint.py @@ -3099,12 +3099,19 @@ def CheckForFunctionCommentHeaders(filename, raw_lines, error): starting_func = False # Look for declaration function_name( but allowing for *, & being attached to the function name # but not being considered part of it - regexp = r'\w(\w|::|\s|\*|\&)* (\*|\&)?(?P\w(\w|::)*)\(' + regexp = r'\w(\w|::|\s|\*|\&)* (\*|\&)?(?P(\w(\w|::)*))\('# + operator_regexp = r'\w(\w|::|\s|\*|\&)* (\*|\&)?(?P(|operator\(.*\)|operator.*))\(' + operator_match = Match(operator_regexp, line) match_result = Match(regexp, line) - if match_result: + function_name = "" + if operator_match: + function_name = operator_match.group('fnc_name') + elif match_result: + function_name = match_result.group('fnc_name') + + if operator_match or match_result: # If the name is all caps and underscores, figure it's a macro and # ignore it, unless it's TEST or TEST_F. - function_name = match_result.group('fnc_name') if function_name == 'TEST' or function_name == 'TEST_F' or ( not Match(r'[A-Z_]+$', function_name)): starting_func = True @@ -3157,6 +3164,8 @@ def CheckForFunctionCommentHeader(filename, raw_lines, linenum, function_name, e """ + function_name = re.escape(function_name) + header_top_regex = r'^/\*{67}\\$' header_bottom_regex = r'^\\\*{67}/$' function_name_regex = r'Function: (\w+::)?' + function_name+'$' From d590db4127c06e9a2eced67d1a43846d1fbc03d9 Mon Sep 17 00:00:00 2001 From: thk123 Date: Thu, 22 Dec 2016 16:42:26 +0000 Subject: [PATCH 30/30] Don't think function declarations should have function header comments Previously we would always checkf for function header comments if we found a function. Now if we find a ; before a { then we assume it is a function declaration, otherwise we assume it is a function implementation and therefore should have the function header comment. This does mean that functions with bodies in header files are going to require a function header comment. --- .../function-comment-header7/main.cpp | 15 +++++++++++ .../function-comment-header7/test.desc | 7 ++++++ scripts/cpplint.py | 25 ++++++------------- 3 files changed, 29 insertions(+), 18 deletions(-) create mode 100644 regression/cpp-linter/function-comment-header7/main.cpp create mode 100644 regression/cpp-linter/function-comment-header7/test.desc diff --git a/regression/cpp-linter/function-comment-header7/main.cpp b/regression/cpp-linter/function-comment-header7/main.cpp new file mode 100644 index 00000000000..69af8182df5 --- /dev/null +++ b/regression/cpp-linter/function-comment-header7/main.cpp @@ -0,0 +1,15 @@ +/*******************************************************************\ + +Module: Lint Examples + +Author: Thomas Kiley, thomas@diffblue.com + +\*******************************************************************/ + + + +static void fun( + bool param1, + bool param2); + +static void bar(); diff --git a/regression/cpp-linter/function-comment-header7/test.desc b/regression/cpp-linter/function-comment-header7/test.desc new file mode 100644 index 00000000000..12418d892f4 --- /dev/null +++ b/regression/cpp-linter/function-comment-header7/test.desc @@ -0,0 +1,7 @@ +CORE +main.cpp + +^Total errors found: 0$ +^EXIT=0$ +^SIGNAL=0$ +-- diff --git a/scripts/cpplint.py b/scripts/cpplint.py index 39332ab3790..20f4b4b3fce 100644 --- a/scripts/cpplint.py +++ b/scripts/cpplint.py @@ -3120,26 +3120,15 @@ def CheckForFunctionCommentHeaders(filename, raw_lines, error): body_found = False for start_linenum in xrange(linenum, len(raw_lines)): start_line = raw_lines[start_linenum] - joined_line += ' ' + start_line.lstrip() - if Search(r'(;|})', start_line): # Declarations and trivial functions + if Search(r'{', start_line): body_found = True - break # ... ignore - elif Search(r'{', start_line): - body_found = True - function = Search(r'((\w|:)*)\(', line).group(1) - if Match(r'TEST', function): # Handle TEST... macros - parameter_regexp = Search(r'(\(.*\))', joined_line) - if parameter_regexp: # Ignore bad syntax - function += parameter_regexp.group(1) - else: - function += '()' - function_state.Begin(function) break - if not body_found: - # No body for the function (or evidence of a non-function) was found. - error(filename, linenum, 'readability/fn_size', 5, - 'Lint failed to find start of function body.') - else: + elif Search(r';', start_line): + body_found = False + break + + # body found, i.e. not a declaration + if body_found: CheckForFunctionCommentHeader(filename, raw_lines, linenum, function_name, error) linenum += 1