From 6193f95bb5109433628dbf0939ac7b3efd5ebfa0 Mon Sep 17 00:00:00 2001 From: thk123 Date: Thu, 5 Jan 2017 14:30:48 +0000 Subject: [PATCH 1/6] Correctly deal with non-t terminated type names The old way of checking for classes ended in t was confusing as used a negative match. As such it was difficult to check that it ended in a t but count & or * as the terminator of the type. We start the search at a bounardy to deal with classes inside brackets but not when class is part of a larger word. Instead we now just try and match the name since this will greedly consume the whole name and nothing more. The template check also uses boundary rather than the start line anchor since sometimes template statements can be indendented. However in these cases we still don't want to check the type ends in a t. This stops us incorrectly reporting types not ending in t as we don't erronousely treat other chartacers as the final charatcer of the type. Added some extra tests to deal with the variety of ways that the class keyword can appear in the codebase. --- .../cpp-linter/class-decl-space/main.cpp | 33 +++++++++++++++++++ .../cpp-linter/class-decl-space/test.desc | 9 ++++- .../cpp-linter/struct-inline-decl/main.cpp | 24 ++++++++++++++ .../cpp-linter/struct-inline-decl/test.desc | 8 +++++ scripts/cpplint.py | 14 +++++--- 5 files changed, 83 insertions(+), 5 deletions(-) create mode 100644 regression/cpp-linter/struct-inline-decl/main.cpp create mode 100644 regression/cpp-linter/struct-inline-decl/test.desc diff --git a/regression/cpp-linter/class-decl-space/main.cpp b/regression/cpp-linter/class-decl-space/main.cpp index 97c7db3a835..6f0ce90641e 100644 --- a/regression/cpp-linter/class-decl-space/main.cpp +++ b/regression/cpp-linter/class-decl-space/main.cpp @@ -8,3 +8,36 @@ Author: Thomas Kiley, thomas@diffblue.com class temp_classt : public base_classt {} + +class another_class : public base_classt +{} + +class more_class:public base_classt +{} + +class nonderived +{} + +class nonderivedt +{} + +#define ID_property_class dstring(23, 0) + +int foo(class define); + +int foo(class definet); + +template +void bar(U t); + +template +void bar(U t); + +template < class U > +void bar(U t); + +class testt +{ + template + void bar(U t); +} diff --git a/regression/cpp-linter/class-decl-space/test.desc b/regression/cpp-linter/class-decl-space/test.desc index 87b8d462144..c3e5fd574b4 100644 --- a/regression/cpp-linter/class-decl-space/test.desc +++ b/regression/cpp-linter/class-decl-space/test.desc @@ -2,6 +2,13 @@ CORE main.cpp ^main\.cpp:9: There shouldn.t be a space between class identifier and : \[readability/identifiers\] \[4\]$ -^Total errors found: 1$ +^main\.cpp:12: There shouldn.t be a space between class identifier and : \[readability/identifiers\] \[4\]$ +^main\.cpp:12: Class or struct identifier should end with t \[readability/identifiers\] \[4\]$ +^main\.cpp:15: Class or struct identifier should end with t \[readability/identifiers\] \[4\]$ +^main\.cpp:18: Class or struct identifier should end with t \[readability/identifiers\] \[4\]$ +^main\.cpp:26: Class or struct identifier should end with t \[readability/identifiers\] \[4\]$ +^main\.cpp:30: Remove spaces around < \[whitespace/operators\] \[4\]$ +^main\.cpp:36: Remove spaces around < \[whitespace/operators\] \[4\]$ +^Total errors found: 8$ ^SIGNAL=0$ -- diff --git a/regression/cpp-linter/struct-inline-decl/main.cpp b/regression/cpp-linter/struct-inline-decl/main.cpp new file mode 100644 index 00000000000..7676ae04912 --- /dev/null +++ b/regression/cpp-linter/struct-inline-decl/main.cpp @@ -0,0 +1,24 @@ +/*******************************************************************\ + +Module: Lint Examples + +Author: Thomas Kiley, thomas@diffblue.com + +\*******************************************************************/ + +/*******************************************************************\ + +Function: fun + + Inputs: + + Outputs: + + Purpose: + +\*******************************************************************/ + +static void fun() +{ + bar(struct mystructt& struct_var); +} diff --git a/regression/cpp-linter/struct-inline-decl/test.desc b/regression/cpp-linter/struct-inline-decl/test.desc new file mode 100644 index 00000000000..4a53c26c870 --- /dev/null +++ b/regression/cpp-linter/struct-inline-decl/test.desc @@ -0,0 +1,8 @@ +CORE +main.cpp + + +^Total errors found: 0$ +^EXIT=0$ +^SIGNAL=0$ +-- diff --git a/scripts/cpplint.py b/scripts/cpplint.py index 20f4b4b3fce..1186d6b09da 100644 --- a/scripts/cpplint.py +++ b/scripts/cpplint.py @@ -3562,10 +3562,16 @@ def CheckOperatorSpacing(filename, clean_lines, linenum, error): 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](;$|\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') + #check type definitions end with t + # Look for class declarations and check the final character is a t + # Exclude classes in side template argument lists (why?) + class_name_match = Search(r'\b(struct|class)\s(?P[\w_]+)', line) + if class_name_match: + class_name = class_name_match.group('class_name') + if not class_name.endswith('t'): + if not Search(r'\btemplate <', line) and not Search(r'\btemplate<', line): + error(filename, linenum, 'readability/identifiers', 4, + 'Class or struct identifier should end with t') if Search(r'(struct|class)\s[\w_]*_t(;$|\s|:|$)', line): error(filename, linenum, 'readability/identifiers', 4, From 7b27259cf41e904b4b825c28ccccf304541f74ff Mon Sep 17 00:00:00 2001 From: thk123 Date: Thu, 5 Jan 2017 14:52:03 +0000 Subject: [PATCH 2/6] Remove redundant code This result is legacy from some debugging so removing (already merged into master so can't be rebased) --- scripts/cpplint.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/scripts/cpplint.py b/scripts/cpplint.py index 1186d6b09da..b6d1dac2345 100644 --- a/scripts/cpplint.py +++ b/scripts/cpplint.py @@ -3530,9 +3530,6 @@ def CheckOperatorSpacing(filename, clean_lines, linenum, error): 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? From 8308fbfc5ea53d61b5cfbbc227ace3fce3ada361 Mon Sep 17 00:00:00 2001 From: thk123 Date: Thu, 5 Jan 2017 14:53:03 +0000 Subject: [PATCH 3/6] Renamed variables for space check The match result variables from the regex had unclear names. Made the names more explicit. --- scripts/cpplint.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/scripts/cpplint.py b/scripts/cpplint.py index b6d1dac2345..798736c83f8 100644 --- a/scripts/cpplint.py +++ b/scripts/cpplint.py @@ -3525,12 +3525,12 @@ 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]+[\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) + left_hand_space_match = Search(r'[^\s]+[\s](=|>=|<=|==|!=|&=|\^=|\|=|\+=|\*=|\/=|\%=|>|<|\^|\+[^\+]|\/)', line) + right_hand_space_match = Search(r'(=|>=|<=|==|!=|&=|\^=|\|=|\+=|\*=|\/=|\%=|>|<|!|\^|\+|\/)[\s]', line) + operator_pos, op_end = left_hand_space_match.span(1) if left_hand_space_match else (-1, -1) + operator_pos2, op_end2 = right_hand_space_match.span(1) if right_hand_space_match else (-1, -1) - if (match1 and + if (left_hand_space_match 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?? @@ -3542,12 +3542,12 @@ def CheckOperatorSpacing(filename, clean_lines, linenum, error): # and not Search(r'operator=', line)): error(filename, linenum, 'whitespace/operators', 4, # 'Missing spaces around =') - 'Remove spaces around %s' % match1.group(1)) - elif (match2 and + 'Remove spaces around %s' % left_hand_space_match.group(1)) + elif (right_hand_space_match 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)) + 'Remove spaces around %s' % right_hand_space_match.group(0)) # these would cause too many false alarms if we checked for one-sided spaces only match = Search(r'\s(-|\*)(\s|$)', line) From 47106fb4dfa96acf45d6382a1367bcf5b714863e Mon Sep 17 00:00:00 2001 From: thk123 Date: Thu, 5 Jan 2017 15:51:47 +0000 Subject: [PATCH 4/6] Removed check for << Previously we were only allowing << around digits, but this produces false positives as there are plenty of uses of the shift operator that use variables. At this stage, I believe it is better to just remove this check. --- regression/cpp-linter/operator-spacing2/main.cpp | 3 +++ regression/cpp-linter/operator-spacing2/test.desc | 4 ++-- regression/cpp-linter/operator-spacing3/main.cpp | 2 ++ scripts/cpplint.py | 11 ----------- 4 files changed, 7 insertions(+), 13 deletions(-) diff --git a/regression/cpp-linter/operator-spacing2/main.cpp b/regression/cpp-linter/operator-spacing2/main.cpp index 90db55a167e..66567aba343 100644 --- a/regression/cpp-linter/operator-spacing2/main.cpp +++ b/regression/cpp-linter/operator-spacing2/main.cpp @@ -24,8 +24,11 @@ static void fun() int x = 1<<4; + // Ideally this should produce an error, see operator-spacing3 status()<<"Adding CPROVER library ("<hash())-result; } diff --git a/scripts/cpplint.py b/scripts/cpplint.py index 798736c83f8..5b6d6994d54 100644 --- a/scripts/cpplint.py +++ b/scripts/cpplint.py @@ -3633,17 +3633,6 @@ def CheckOperatorSpacing(filename, clean_lines, linenum, error): # error(filename, linenum, 'whitespace/operators', 3, # 'Missing spaces around >') - # We allow no-spaces around << when used like this: 10<<20, but - # not otherwise (particularly, not when used as streams) - # - # We also allow operators following an opening parenthesis, since - # those tend to be macros that deal with operators. - match = Search(r'(operator|[^\s(<])(?:L|UL|LL|ULL|l|ul|ll|ull)?<<([^\s,=<])', line) - if (match and not (match.group(1).isdigit() and match.group(2).isdigit()) and - not (match.group(1) == 'operator' and match.group(2) == ';')): - error(filename, linenum, 'whitespace/operators', 3, - 'Missing spaces around <<') - # We allow no-spaces around >> for almost anything. This is because # C++11 allows ">>" to close nested templates, which accounts for # most cases when ">>" is not followed by a space. From 14f2a98dd9fc1780cc8a86cad4fd73cd6f6d1ef7 Mon Sep 17 00:00:00 2001 From: thk123 Date: Thu, 5 Jan 2017 15:55:19 +0000 Subject: [PATCH 5/6] Removed erroneous sentence about constant array variable names We keep the check to avoid using variable length arrays. However, the warming about camel cased constants begining with k is not relevant. --- scripts/cpplint.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/scripts/cpplint.py b/scripts/cpplint.py index 5b6d6994d54..d3cbcdfdfa1 100644 --- a/scripts/cpplint.py +++ b/scripts/cpplint.py @@ -5080,8 +5080,7 @@ def CheckLanguage(filename, clean_lines, linenum, file_extension, break if not is_const: error(filename, linenum, 'runtime/arrays', 1, - 'Do not use variable-length arrays. Use an appropriately named ' - "('k' followed by CamelCase) compile-time constant for the size.") + 'Do not use variable-length arrays.') # Check for use of unnamed namespaces in header files. Registration # macros are typically OK, so we allow use of "namespace {" on lines From 548195f084dd1305d5d08cac688db4ec200962ad Mon Sep 17 00:00:00 2001 From: thk123 Date: Thu, 5 Jan 2017 17:36:06 +0000 Subject: [PATCH 6/6] Added check for position of &/* for types We attempt to validate that the */& for pointer/reference types are attached to the variable rather than the type. For example should be int *x; rather than int* x; We attempt to check this by looking for things that are at the very start of the line and not in brackets to avoid picking up cases where the * or & is used as a binary operator. We can still miss cases inside a function declaration. Updated the coding standard to also explicityl mention this preference --- CODING_STANDARD | 3 + regression/cpp-linter/pointer-type1/main.cpp | 37 ++++++++ regression/cpp-linter/pointer-type1/test.desc | 8 ++ scripts/cpplint.py | 92 ++++++++++++++++--- 4 files changed, 128 insertions(+), 12 deletions(-) create mode 100644 regression/cpp-linter/pointer-type1/main.cpp create mode 100644 regression/cpp-linter/pointer-type1/test.desc diff --git a/CODING_STANDARD b/CODING_STANDARD index 3466cd05c99..41b9bdd8679 100644 --- a/CODING_STANDARD +++ b/CODING_STANDARD @@ -17,6 +17,9 @@ Whitespaces: Exceptions: Spaces around &&, || and << - Space after comma (parameter lists, argument lists, ...) - Space after colon inside 'for' +- For pointers and references, the */& should be attached to the variable name + as oppposed to the tyep. E.g. for a pointer to an int the syntax would be: + `int *x;` - No whitespaces at end of line - No whitespaces in blank lines - Put argument lists on next line (and ident 2 spaces) if too long diff --git a/regression/cpp-linter/pointer-type1/main.cpp b/regression/cpp-linter/pointer-type1/main.cpp new file mode 100644 index 00000000000..dabe24e7dc6 --- /dev/null +++ b/regression/cpp-linter/pointer-type1/main.cpp @@ -0,0 +1,37 @@ +/*******************************************************************\ + +Module: Lint Examples + +Author: Thomas Kiley, thomas@diffblue.com + +\*******************************************************************/ + +/*******************************************************************\ + +Function: fun + + Inputs: + + Outputs: + + Purpose: + +\*******************************************************************/ + +static void fun() +{ + bool *x=nullptr; // Valid + bool* x=nullptr; // Invalid + + int &x=nullptr; // Valid + int& x=nullptr; // Invalid + + int y=at*bt; // Valid + + // Probably valid - could be a pointer to type yt or a + // variable called yt multilied by z. Would have to know + // it is a function call rather than a function declaration + foo( + x, + yt*z); +} diff --git a/regression/cpp-linter/pointer-type1/test.desc b/regression/cpp-linter/pointer-type1/test.desc new file mode 100644 index 00000000000..548cc52eee1 --- /dev/null +++ b/regression/cpp-linter/pointer-type1/test.desc @@ -0,0 +1,8 @@ +CORE +main.cpp + +^main\.cpp:24: Pointer type name must have \* attached to the type name \[whitespace/operators\] \[4\]$ +^main\.cpp:27: Reference type name must have & attached to the type name \[whitespace/operators\] \[4\]$ +^Total errors found: 2$ +^SIGNAL=0$ +-- diff --git a/scripts/cpplint.py b/scripts/cpplint.py index d3cbcdfdfa1..e4d7916f6e3 100644 --- a/scripts/cpplint.py +++ b/scripts/cpplint.py @@ -1547,20 +1547,17 @@ def IsTemplateArgumentList_DB(clean_lines, linenum, pos): 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 +def ForceOpenExpression(clean_lines, linenum, pos, bracket): + """Find an opening bracket matching the specified closing bracket - TODO(tkiley): could probably be merged with CloseExpression + Search starting at the position for a matching closing bracket of the + same type as bracket. Args: clean_lines: A CleansedLines instance containing the file. linenum: The number of the line to check. pos: A position on the line. + bracket: The style of bracket to match Returns: A tuple (line, linenum, pos) pointer *to* the closing brace, or @@ -1568,13 +1565,10 @@ def OpenExpression(clean_lines, linenum, pos): 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 , []) + (end_pos, stack) = FindStartOfExpressionInLine(line, pos , [bracket]) if end_pos > -1: return (line, linenum, end_pos) @@ -1589,6 +1583,37 @@ def OpenExpression(clean_lines, linenum, pos): # Did not find end of expression before end of file, give up return (line, clean_lines.NumLines(), -1) +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 + + Calls ForceOpenExpression with the bracket type pointed at + + 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) + else: + return ForceOpenExpression(clean_lines, linenum, pos-1, line[pos]) + + def FindEndOfExpressionInLine(line, startpos, stack): """Find the position just after the end of current parenthesized expression. @@ -3656,6 +3681,48 @@ def CheckOperatorSpacing(filename, clean_lines, linenum, error): error(filename, linenum, 'whitespace/operators', 4, 'Extra space for operator %s' % match.group(1)) +def CheckPointerReferenceSpacing(filename, clean_lines, linenum, error): + """Checks the */& are attached to variable names rather than types + + A pointer or reference type should have the */& attached to the variable + name rather than the type. I.e. a pointer to an int should be: + + int *var_name; + + 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] + + # Find types by looking for word_names that are at the start of the line + # If there are followed by a * or & (after an optional ' const') + # then they appear to be a reference/pointer type with it attached to + # the type rather than the variable + wrong_type_match = Search(r'^\s*([\w_])+( const)?(?P[&\*])\s*\w', line) + + if wrong_type_match: + # This still could be a false positive, we must + # check that we are not inside brackets as then could be just be + # operators (multiply and logical AND) + pos = wrong_type_match.start(1) + _, _, opening_pos = ForceOpenExpression(clean_lines, linenum, pos, ')') + + # If we don't find a matching close brace then we aren't in brackets + # so can assume this is a type with the * or & attached to it + if opening_pos == -1: + op_type = wrong_type_match.group('type') + op_word = "" + if op_type == '*': + op_word = 'Pointer' + else: + op_word = 'Reference' + error(filename, linenum, 'whitespace/operators', 4, + op_word + ' type name must have ' + op_type + ' attached to the type name') + + def CheckParenthesisSpacing(filename, clean_lines, linenum, error): """Checks for horizontal spacing around parentheses. @@ -4671,6 +4738,7 @@ def CheckStyle(filename, clean_lines, linenum, file_extension, nesting_state, CheckAccess(filename, clean_lines, linenum, nesting_state, error) CheckSpacing(filename, clean_lines, linenum, nesting_state, error) CheckOperatorSpacing(filename, clean_lines, linenum, error) + CheckPointerReferenceSpacing(filename, clean_lines, linenum, error) CheckParenthesisSpacing(filename, clean_lines, linenum, error) CheckCommaSpacing(filename, clean_lines, linenum, error) CheckBracesSpacing(filename, clean_lines, linenum, nesting_state, error)