diff --git a/wpiformat/test/test_virtualspecifier.py b/wpiformat/test/test_virtualspecifier.py new file mode 100644 index 00000000..e2b2288d --- /dev/null +++ b/wpiformat/test/test_virtualspecifier.py @@ -0,0 +1,107 @@ +import os + +from wpiformat.config import Config +from wpiformat.virtualspecifier import VirtualSpecifier + + +def test_virtualspecifier(): + task = VirtualSpecifier() + + inputs = [] + outputs = [] + + # Redundant virtual specifier on function + inputs.append(("./PWM.h", + "class PWM : public SendableBase {" + os.linesep + \ + " public:" + os.linesep + \ + " explicit PWM(int channel);" + os.linesep + \ + " ~PWM() override;" + os.linesep + \ + os.linesep + \ + " protected:" + os.linesep + \ + " virtual void InitSendable(SendableBuilder& builder) override;" + os.linesep + \ + "};" + os.linesep)) + outputs.append(( + "class PWM : public SendableBase {" + os.linesep + \ + " public:" + os.linesep + \ + " explicit PWM(int channel);" + os.linesep + \ + " ~PWM() override;" + os.linesep + \ + os.linesep + \ + " protected:" + os.linesep + \ + " void InitSendable(SendableBuilder& builder) override;" + os.linesep + \ + "};" + os.linesep, True, True)) + + # Redundant virtual specifier on const function + inputs.append(("./PIDController.h", + "class PIDController : public PIDInterface {" + os.linesep + \ + " public:" + os.linesep + \ + " virtual double GetP() const override;" + os.linesep + \ + " virtual double GetI() const override;" + os.linesep + \ + " virtual double GetD() const override;" + os.linesep + \ + "};" + os.linesep)) + outputs.append(( + "class PIDController : public PIDInterface {" + os.linesep + \ + " public:" + os.linesep + \ + " double GetP() const override;" + os.linesep + \ + " double GetI() const override;" + os.linesep + \ + " double GetD() const override;" + os.linesep + \ + "};" + os.linesep, True, True)) + + # Redundant final specifier on const function + inputs.append(("./PIDController.h", + "class PIDController : public PIDInterface {" + os.linesep + \ + " public:" + os.linesep + \ + " double GetP() const override;" + os.linesep + \ + " double GetI() const final override;" + os.linesep + \ + " double GetD() const override final;" + os.linesep + \ + "};" + os.linesep)) + outputs.append(( + "class PIDController : public PIDInterface {" + os.linesep + \ + " public:" + os.linesep + \ + " double GetP() const override;" + os.linesep + \ + " double GetI() const final;" + os.linesep + \ + " double GetD() const final;" + os.linesep + \ + "};" + os.linesep, True, True)) + + # Redundant virtual specifier on destructor + inputs.append(("./PWM.h", + "class PWM : public SendableBase {" + os.linesep + \ + " public:" + os.linesep + \ + " explicit PWM(int channel);" + os.linesep + \ + " virtual ~PWM() override;" + os.linesep + \ + os.linesep + \ + " virtual void SetRaw(uint16_t value);" + os.linesep + \ + "};" + os.linesep)) + outputs.append(( + "class PWM : public SendableBase {" + os.linesep + \ + " public:" + os.linesep + \ + " explicit PWM(int channel);" + os.linesep + \ + " ~PWM() override;" + os.linesep + \ + os.linesep + \ + " virtual void SetRaw(uint16_t value);" + os.linesep + \ + "};" + os.linesep, True, True)) + + # Idempotence with virtual specifier on destructor + inputs.append(("./PWM.h", + "class PWM : public SendableBase {" + os.linesep + \ + " public:" + os.linesep + \ + " explicit PWM(int channel);" + os.linesep + \ + " virtual ~PWM();" + os.linesep + \ + "};" + os.linesep)) + outputs.append(( + "class PWM : public SendableBase {" + os.linesep + \ + " public:" + os.linesep + \ + " explicit PWM(int channel);" + os.linesep + \ + " virtual ~PWM();" + os.linesep + \ + "};" + os.linesep, False, True)) + + assert len(inputs) == len(outputs) + + config_file = Config(os.path.abspath(os.getcwd()), ".styleguide") + for i in range(len(inputs)): + print("TEST {}".format(i)) + output, file_changed, success = task.run_pipeline( + config_file, inputs[i][0], inputs[i][1]) + print("TEST {} check".format(i)) + assert output == outputs[i][0] + assert file_changed == outputs[i][1] + assert success == outputs[i][2] diff --git a/wpiformat/wpiformat/__init__.py b/wpiformat/wpiformat/__init__.py index e6aa6184..659cc3a0 100644 --- a/wpiformat/wpiformat/__init__.py +++ b/wpiformat/wpiformat/__init__.py @@ -23,6 +23,7 @@ from wpiformat.task import Task from wpiformat.usingdeclaration import UsingDeclaration from wpiformat.usingnamespacestd import UsingNamespaceStd +from wpiformat.virtualspecifier import VirtualSpecifier from wpiformat.whitespace import Whitespace @@ -339,6 +340,7 @@ def main(): IncludeOrder(), UsingDeclaration(), UsingNamespaceStd(), + VirtualSpecifier(), Whitespace() ] run_pipeline(task_pipeline, args, files) diff --git a/wpiformat/wpiformat/cpplint.py b/wpiformat/wpiformat/cpplint.py index 9f14fc88..a206cb04 100644 --- a/wpiformat/wpiformat/cpplint.py +++ b/wpiformat/wpiformat/cpplint.py @@ -3446,99 +3446,6 @@ def CheckMakePairUsesDeduction(filename, clean_lines, linenum, error): ' OR use pair directly OR if appropriate, construct a pair directly') -def CheckRedundantVirtual(filename, clean_lines, linenum, error): - """Check if line contains a redundant "virtual" function-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 "virtual" on current line. - line = clean_lines.elided[linenum] - virtual = Match(r'^(.*)(\bvirtual\b)(.*)$', line) - if not virtual: return - - # Ignore "virtual" keywords that are near access-specifiers. These - # are only used in class base-specifier and do not apply to member - # functions. - if (Search(r'\b(public|protected|private)\s+$', virtual.group(1)) or - Match(r'^\s+(public|protected|private)\b', virtual.group(3))): - return - - # Ignore the "virtual" keyword from virtual base classes. Usually - # there is a column on the same line in these cases (virtual base - # classes are rare in google3 because multiple inheritance is rare). - if Match(r'^.*[^:]:[^:].*$', line): return - - # Look for the next opening parenthesis. This is the start of the - # parameter list (possibly on the next line shortly after virtual). - # TODO(unknown): doesn't work if there are virtual functions with - # decltype() or other things that use parentheses, but csearch suggests - # that this is rare. - end_col = -1 - end_line = -1 - start_col = len(virtual.group(2)) - for start_line in range(linenum, min(linenum + 3, clean_lines.NumLines())): - line = clean_lines.elided[start_line][start_col:] - parameter_list = Match(r'^([^(]*)\(', line) - if parameter_list: - # Match parentheses to find the end of the parameter list - (_, end_line, end_col) = CloseExpression( - clean_lines, start_line, start_col + len(parameter_list.group(1))) - break - start_col = 0 - - if end_col < 0: - return # Couldn't find end of parameter list, give up - - # Look for "override" or "final" after the parameter list - # (possibly on the next few lines). - for i in range(end_line, min(end_line + 3, clean_lines.NumLines())): - line = clean_lines.elided[i][end_col:] - match = Search(r'\b(override|final)\b', line) - if match: - error(filename, linenum, 'readability/inheritance', 4, - ('"virtual" is redundant since function is ' - 'already declared as "%s"' % match.group(1))) - - # Set end_col to check whole lines after we are done with the - # first line. - end_col = 0 - if Search(r'[^\w]\s*$', line): - 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. def IsBlockInNameSpace(nesting_state, is_forward_declaration): @@ -3590,8 +3497,6 @@ def ProcessLine(filename, is_header, clean_lines, line, nesting_state, error) CheckInvalidIncrement(filename, clean_lines, line, error) CheckMakePairUsesDeduction(filename, clean_lines, line, error) - CheckRedundantVirtual(filename, clean_lines, line, error) - CheckRedundantOverrideOrFinal(filename, clean_lines, line, error) def ProcessFileData(filename, is_header, lines, error): diff --git a/wpiformat/wpiformat/virtualspecifier.py b/wpiformat/wpiformat/virtualspecifier.py new file mode 100644 index 00000000..5beef6e3 --- /dev/null +++ b/wpiformat/wpiformat/virtualspecifier.py @@ -0,0 +1,76 @@ +"""This task removes redundant "virtual" specifier instances. + +When the "override" specifier is specified in a C++ function signature, the +"virtual" specifier is redundant because "override" implies "virtual". +""" + +import regex + +from wpiformat.task import Task + + +class VirtualSpecifier(Task): + + def should_process_file(self, config_file, name): + return config_file.is_cpp_file(name) + + def run_pipeline(self, config_file, name, lines): + linesep = Task.get_linesep(lines) + + file_changed = False + output = "" + + # Function signature parts + virtual_spec = "(?Pvirtual\s+)?" + return_type = "(?P[\w,\*&]+\s+)" + func_args = "(?P\([^\)]*\)(\s*const)?)" + specifiers = "(?P[^;{]*)?" + + # Construct regexes for function signatures + regexes = [] + regexes.append( + regex.compile( + virtual_spec + "(?P" + return_type + + "(?P\w+\s*)" + func_args + ")" + specifiers)) + regexes.append( + regex.compile( + virtual_spec + "(?P" + "(?P[\w~]+\s*)" + + func_args + ")" + specifiers)) + + for rgx in regexes: + pos = 0 + for match in rgx.finditer(lines): + # Append lines before match + output += lines[pos:match.start()] + + # Append virtual specifier if it's not redundant + if "final" not in match.group( + "specifiers") and "override" not in match.group( + "specifiers"): + if match.group("virtual"): + output += match.group("virtual") + else: + file_changed = True + output += match.group("func_sig") + + # Strip redundant specifiers + specifiers_in = match.group("specifiers") + specifiers_out = specifiers_in + if "final" in specifiers_out: + specifiers_out = regex.sub("\s+override", "", + specifiers_out) + if specifiers_in != specifiers_out: + file_changed = True + output += specifiers_out + + pos = match.end() + + # Append leftover lines in file + if pos < len(lines): + output += lines[pos:] + + # Reset line buffer for next regex + lines = output + output = "" + + return (lines, file_changed, True)