From eb4597d04a8d5719b19ac66f13fd032b6715875f Mon Sep 17 00:00:00 2001 From: thk123 Date: Mon, 9 Jan 2017 17:55:26 +0000 Subject: [PATCH 1/4] Replaced regex for matching multi-line function calls Instead use a more complex system that finds the matching closing bracket and checks all the lines in between to check one parameter only on each line. It handles nested function calls by skipping over their whole parameter list (since we don't require nested functions be separated onto individual lines just because the outer call is. Instead these get checked when we get to this function call separately. --- CODING_STANDARD | 2 + .../multi-line-function-call1/main.cpp | 79 +++++++++++++++++++ .../multi-line-function-call1/test.desc | 15 ++++ .../multi-line-function-call2/main.cpp | 67 ++++++++++++++++ .../multi-line-function-call2/test.desc | 11 +++ .../multi-line-function-call3/main.cpp | 47 +++++++++++ .../multi-line-function-call3/test.desc | 7 ++ scripts/cpplint.py | 72 ++++++++++++++++- 8 files changed, 297 insertions(+), 3 deletions(-) create mode 100644 regression/cpp-linter/multi-line-function-call1/main.cpp create mode 100644 regression/cpp-linter/multi-line-function-call1/test.desc create mode 100644 regression/cpp-linter/multi-line-function-call2/main.cpp create mode 100644 regression/cpp-linter/multi-line-function-call2/test.desc create mode 100644 regression/cpp-linter/multi-line-function-call3/main.cpp create mode 100644 regression/cpp-linter/multi-line-function-call3/test.desc diff --git a/CODING_STANDARD b/CODING_STANDARD index 41b9bdd8679..6bb51d2119b 100644 --- a/CODING_STANDARD +++ b/CODING_STANDARD @@ -11,6 +11,8 @@ Whitespaces: - 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 + - Nested function calls do not need to be broken up into separate lines even + if the outer function call does. - If a method is bigger than 50 lines, break it into parts. - Put matching { } into the same column. - No spaces around operators (=, +, ==, ...) diff --git a/regression/cpp-linter/multi-line-function-call1/main.cpp b/regression/cpp-linter/multi-line-function-call1/main.cpp new file mode 100644 index 00000000000..1cf221de8d8 --- /dev/null +++ b/regression/cpp-linter/multi-line-function-call1/main.cpp @@ -0,0 +1,79 @@ +/*******************************************************************\ + +Module: Lint Examples + +Author: Thomas Kiley, thomas@diffblue.com + +\*******************************************************************/ + +/*******************************************************************\ + +Function: fun + + Inputs: + + Outputs: + + Purpose: + +\*******************************************************************/ + +static void fun() +{ + // Correct + foo( + x, + y); + + // Incorrect, x should be on the next line + foo(x, + y); + + // Incorrect indent should be only 2 + foo( + x, + y); + + // Correct + int x=bar( + x, + y); + + // Incorrect, x should be on the next line + int x=bar(x, + y); + + // Incorrect indent should be only 2 + int x=bar( + x, + y); + + // Correct + *model=baz( + x, + y); + + // Incorrect, x should be on the next line + *model=baz(x, + y); + + // Incorrect indent should be only 2 + *model=baz( + x, + y); + + // Correct + var->fun( + x, + y); + + // Incorrect, x should be on the next line + var->fun(x, + y); + + // Incorrect indent should be only 2 + var->fun( + x, + y); +} + diff --git a/regression/cpp-linter/multi-line-function-call1/test.desc b/regression/cpp-linter/multi-line-function-call1/test.desc new file mode 100644 index 00000000000..cb9ea2992ba --- /dev/null +++ b/regression/cpp-linter/multi-line-function-call1/test.desc @@ -0,0 +1,15 @@ +CORE +main.cpp + +^main\.cpp:29: If parameters or arguments require a line break, each parameter should be put on its own line\. \[whitespace/indent\] \[4\] +^main\.cpp:34: Indent of wrapped parenthesized expression or parameter or argument list should be 2 \[whitespace/indent\] \[4\] +^main\.cpp:43: If parameters or arguments require a line break, each parameter should be put on its own line\. \[whitespace/indent\] \[4\] +^main\.cpp:48: Indent of wrapped parenthesized expression or parameter or argument list should be 2 \[whitespace/indent\] \[4\] +^main\.cpp:57: If parameters or arguments require a line break, each parameter should be put on its own line\. \[whitespace/indent\] \[4\] +^main\.cpp:62: Indent of wrapped parenthesized expression or parameter or argument list should be 2 \[whitespace/indent\] \[4\] +^main\.cpp:71: If parameters or arguments require a line break, each parameter should be put on its own line\. \[whitespace/indent\] \[4\] +^main\.cpp:76: Indent of wrapped parenthesized expression or parameter or argument list should be 2 \[whitespace/indent\] \[4\] +^Total errors found: 8$ +^EXIT=1$ +^SIGNAL=0$ +-- diff --git a/regression/cpp-linter/multi-line-function-call2/main.cpp b/regression/cpp-linter/multi-line-function-call2/main.cpp new file mode 100644 index 00000000000..bfa16f58082 --- /dev/null +++ b/regression/cpp-linter/multi-line-function-call2/main.cpp @@ -0,0 +1,67 @@ +/*******************************************************************\ + +Module: Lint Examples + +Author: Thomas Kiley, thomas@diffblue.com + +\*******************************************************************/ + +/*******************************************************************\ + +Function: fun + + Inputs: + + Outputs: + + Purpose: + +\*******************************************************************/ + +static void fun() +{ + // Incorrect, call should be on a new line + nested(call(), + another_param); + + // Incorrect - should be indented by 2 + nested( + call(), + another_param); + + // Correct + nested( + call(), + another_param); + + // Correct + twice( + nested( + call( + param1), + param2), + param3) + + // Correct + foo( + bar(x, y), + z); + + // Correct + foo( + bar( + x, + y), + z); + + // Incorrect, the bar arguments have been split up therefore + // they all should be split up + foo( + bar(x, + y), + z); + + // Incorrect bar should be on the next line + foo(bar(x, y), + z); +} diff --git a/regression/cpp-linter/multi-line-function-call2/test.desc b/regression/cpp-linter/multi-line-function-call2/test.desc new file mode 100644 index 00000000000..2fb7df0f981 --- /dev/null +++ b/regression/cpp-linter/multi-line-function-call2/test.desc @@ -0,0 +1,11 @@ +CORE +main.cpp + +^main\.cpp:24: If parameters or arguments require a line break, each parameter should be put on its own line\. \[whitespace/indent\] \[4\] +^main\.cpp:29: Indent of wrapped parenthesized expression or parameter or argument list should be 2 \[whitespace/indent\] \[4\] +^main\.cpp:60: If parameters or arguments require a line break, each parameter should be put on its own line\. \[whitespace/indent\] \[4\] +^main\.cpp:65: If parameters or arguments require a line break, each parameter should be put on its own line\. \[whitespace/indent\] \[4\] +^Total errors found: 4$ +^EXIT=1$ +^SIGNAL=0$ +-- diff --git a/regression/cpp-linter/multi-line-function-call3/main.cpp b/regression/cpp-linter/multi-line-function-call3/main.cpp new file mode 100644 index 00000000000..84f0bcc1ba5 --- /dev/null +++ b/regression/cpp-linter/multi-line-function-call3/main.cpp @@ -0,0 +1,47 @@ +/*******************************************************************\ + +Module: Lint Examples + +Author: Thomas Kiley, thomas@diffblue.com + +\*******************************************************************/ + +/*******************************************************************\ + +Function: fun + + Inputs: + + Outputs: + + Purpose: + +\*******************************************************************/ + +static void fun() +{ + // Correct as for loop not function + for(int x=0; + x<10; + ++x) + {} + + // Correct as an if statement not a function + if(a==b || + c==d) + { + do_something(); + } + + // Correct as ranged based for loop not function + for(x: + list) + {} + + // Correct since if statement + if(some_check(with, params)== + some_value) + { + do_something(); + } +} diff --git a/regression/cpp-linter/multi-line-function-call3/test.desc b/regression/cpp-linter/multi-line-function-call3/test.desc new file mode 100644 index 00000000000..12418d892f4 --- /dev/null +++ b/regression/cpp-linter/multi-line-function-call3/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 e4d7916f6e3..07114bb690c 100644 --- a/scripts/cpplint.py +++ b/scripts/cpplint.py @@ -4686,9 +4686,75 @@ def CheckStyle(filename, clean_lines, linenum, file_extension, nesting_state, 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.') + # here a regex isn't sufficent we need a stack to match brackets + # because even an open bracket and a , at the end might just be function call + # as a parameter. + # Instead the rule we try to apply here is: + # - if we find an opening bracket, we find the matching closing bracket + # - if the bracket is on a different line we require all of the parameters to be on a separate line + # - if there is another opening bracket Skip to the closing bracket as will be checked in a subsequent line check + # - ignore the line if it is a for/if etc since rules are different + + # Look for an opening bracket that doesn't have a semi-colon on the same line + bracket_search = Search(r'(?P\()[^;]*,[^;]*$', elided_line) + + # Exclude the check if any of these keywords are present + # They could trip us up as they have different formatting rules to functions + keyword_search = Search(r'\b(if|for|while|catch|switch)\b', elided_line) + + if bracket_search and not keyword_search: + open_bracket_pos = bracket_search.start('bracket') + close_line, close_linenum, close_pos = CloseExpression(clean_lines, linenum, open_bracket_pos) + if close_pos != -1: + # If the closing line is different from the opening line we need to + # verify that each of the parameters are on separate lines + if close_linenum != linenum: + # The first line needs to have no parameters on it + if(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.') + + # For each line afer we need to verify it consists of exactly one parameter + # Except if we find an opening bracket - in this case we ignore everything until the closing + # bracket (any errors within these brackets will be picked up when we check this line) + start_linenum = linenum + 1 + while(start_linenum < close_linenum): + arg_line = clean_lines.elided[start_linenum] + nested_bracket_search = Search('\(', arg_line) + if nested_bracket_search: + nested_open_bracket_pos = nested_bracket_search.start() + # Just because we are calling a nested function doesn't mean + # we allow multiple parameters on the line + if(Search(',', arg_line[:nested_open_bracket_pos])): + error(filename, start_linenum, 'whitespace/indent', 4, + 'If parameters or arguments require a line break, each parameter should be put on its own line.') + + nested_close_line, nested_close_linenum, _ = CloseExpression(clean_lines, start_linenum, nested_open_bracket_pos) + + # If anything other closing statements or commas appear there is another parameter after the nested call + if not Search(r'\)(,|\)|;)*', nested_close_line): + error(filename, start_linenum, 'whitespace/indent', 4, + 'If parameters or arguments require a line break, each parameter should be put on its own line.') + # Skip to the end of the bracket + start_linenum = nested_close_linenum + + start_linenum+=1 + # For the final line we also need to check one parameter on it + # e.g. we require bracket on same line as last parameter + # foo( + # x); + if not Search(r'^\s*[^,]+\)', close_line): + # If this is true, the we failed because we just had the close bracket + if Search(r'[^,]*\)', close_line): + error(filename, close_linenum, 'whitespace/indent', 4, + 'If parameters or arguments require a line break, the closing bracket should be on the same line as the final parameter') + else: + # In this case the problem is we had a bracket + # i.e. more than one parameter on the last line + error(filename, close_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') From 3c48404dc41d4dbd4977d97bd0ec7aa9e03b230a Mon Sep 17 00:00:00 2001 From: thk123 Date: Tue, 10 Jan 2017 10:19:16 +0000 Subject: [PATCH 2/4] Deal with multi-line function calls with nothing on the first line Previously the regex was checking for the first parameter being on the first line, but this isn't required for the function call to be wrong. Added the check for each of the lines if there is no nested function call. --- regression/cpp-linter/multi-line-function-call1/main.cpp | 9 +++++++++ .../cpp-linter/multi-line-function-call1/test.desc | 4 +++- scripts/cpplint.py | 6 +++++- 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/regression/cpp-linter/multi-line-function-call1/main.cpp b/regression/cpp-linter/multi-line-function-call1/main.cpp index 1cf221de8d8..02383e745ce 100644 --- a/regression/cpp-linter/multi-line-function-call1/main.cpp +++ b/regression/cpp-linter/multi-line-function-call1/main.cpp @@ -75,5 +75,14 @@ static void fun() var->fun( x, y); + + // Incorrect + fun( + x, y); + + // Incorrect + fun( + x, y, + z); } diff --git a/regression/cpp-linter/multi-line-function-call1/test.desc b/regression/cpp-linter/multi-line-function-call1/test.desc index cb9ea2992ba..14cd2f06ff2 100644 --- a/regression/cpp-linter/multi-line-function-call1/test.desc +++ b/regression/cpp-linter/multi-line-function-call1/test.desc @@ -9,7 +9,9 @@ main.cpp ^main\.cpp:62: Indent of wrapped parenthesized expression or parameter or argument list should be 2 \[whitespace/indent\] \[4\] ^main\.cpp:71: If parameters or arguments require a line break, each parameter should be put on its own line\. \[whitespace/indent\] \[4\] ^main\.cpp:76: Indent of wrapped parenthesized expression or parameter or argument list should be 2 \[whitespace/indent\] \[4\] -^Total errors found: 8$ +^main\.cpp:81: If parameters or arguments require a line break, the closing bracket should be on the same line as the final parameter \[whitespace/indent\] \[4\] +^main\.cpp:85: If parameters or arguments require a line break, each parameter should be put on its own line. \[whitespace/indent\] \[4\] +^Total errors found: 10$ ^EXIT=1$ ^SIGNAL=0$ -- diff --git a/scripts/cpplint.py b/scripts/cpplint.py index 07114bb690c..6426fa448a5 100644 --- a/scripts/cpplint.py +++ b/scripts/cpplint.py @@ -4696,7 +4696,7 @@ def CheckStyle(filename, clean_lines, linenum, file_extension, nesting_state, # - ignore the line if it is a for/if etc since rules are different # Look for an opening bracket that doesn't have a semi-colon on the same line - bracket_search = Search(r'(?P\()[^;]*,[^;]*$', elided_line) + bracket_search = Search(r'(?P\()[^;]*$', elided_line) # Exclude the check if any of these keywords are present # They could trip us up as they have different formatting rules to functions @@ -4737,6 +4737,10 @@ def CheckStyle(filename, clean_lines, linenum, file_extension, nesting_state, 'If parameters or arguments require a line break, each parameter should be put on its own line.') # Skip to the end of the bracket start_linenum = nested_close_linenum + else: + if(not Match('^\s*[^,]+,$', arg_line)): + error(filename, start_linenum, 'whitespace/indent', 4, + 'If parameters or arguments require a line break, each parameter should be put on its own line.') start_linenum+=1 # For the final line we also need to check one parameter on it From 648116ce9eddabeca64b87aed8a2ca4652bd02e4 Mon Sep 17 00:00:00 2001 From: thk123 Date: Tue, 10 Jan 2017 10:34:23 +0000 Subject: [PATCH 3/4] Require a word character immediately before space Modified the inital regex to require a word character immediately (except spaces) before the bracket. This ensures we are not just a order of operations bracket but in fact a function call. Added a regression test that checks the case that this is required for --- regression/cpp-linter/multi-line-function-call3/main.cpp | 8 ++++++++ scripts/cpplint.py | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/regression/cpp-linter/multi-line-function-call3/main.cpp b/regression/cpp-linter/multi-line-function-call3/main.cpp index 84f0bcc1ba5..a4bc1918aeb 100644 --- a/regression/cpp-linter/multi-line-function-call3/main.cpp +++ b/regression/cpp-linter/multi-line-function-call3/main.cpp @@ -44,4 +44,12 @@ static void fun() { do_something(); } + + // Correct since if statement + if(condition_a&& + (condition_b|| + condition_c)) + { + do_something(); + } } diff --git a/scripts/cpplint.py b/scripts/cpplint.py index 6426fa448a5..88abdaa5ec2 100644 --- a/scripts/cpplint.py +++ b/scripts/cpplint.py @@ -4696,7 +4696,7 @@ def CheckStyle(filename, clean_lines, linenum, file_extension, nesting_state, # - ignore the line if it is a for/if etc since rules are different # Look for an opening bracket that doesn't have a semi-colon on the same line - bracket_search = Search(r'(?P\()[^;]*$', elided_line) + bracket_search = Search(r'[\w_]+\s*(?P\()[^;]*$', elided_line) # Exclude the check if any of these keywords are present # They could trip us up as they have different formatting rules to functions From 4d98b4308c5d25b41b6d4829f092e1983d5e7084 Mon Sep 17 00:00:00 2001 From: thk123 Date: Tue, 10 Jan 2017 10:41:28 +0000 Subject: [PATCH 4/4] Allow multiple brackets on the opening bracket --- scripts/cpplint.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/cpplint.py b/scripts/cpplint.py index 88abdaa5ec2..6bf84e6528e 100644 --- a/scripts/cpplint.py +++ b/scripts/cpplint.py @@ -4710,7 +4710,7 @@ def CheckStyle(filename, clean_lines, linenum, file_extension, nesting_state, # verify that each of the parameters are on separate lines if close_linenum != linenum: # The first line needs to have no parameters on it - if(Search(r'\(.+', elided_line)): + if(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.')