From a9981eaac59a4b899bb2fcac8affcf228f9281be Mon Sep 17 00:00:00 2001 From: Tyler Veness Date: Mon, 9 Jan 2017 11:21:01 -0800 Subject: [PATCH] Added task for removing redundant "virtual" specifier instances --- wpiformat/wpiformat/__init__.py | 2 + wpiformat/wpiformat/cpplint.py | 95 ------------------- .../wpiformat/test/test_virtualspecifier.py | 94 ++++++++++++++++++ wpiformat/wpiformat/virtualspecifier.py | 76 +++++++++++++++ 4 files changed, 172 insertions(+), 95 deletions(-) create mode 100644 wpiformat/wpiformat/test/test_virtualspecifier.py create mode 100644 wpiformat/wpiformat/virtualspecifier.py diff --git a/wpiformat/wpiformat/__init__.py b/wpiformat/wpiformat/__init__.py index c98a85b4..f4995c52 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 @@ -353,6 +354,7 @@ def main(): IncludeOrder(), UsingDeclaration(), UsingNamespaceStd(), + VirtualSpecifier(), Whitespace(), ClangFormat(args.clang_version), Jni(), # Fixes clang-format formatting diff --git a/wpiformat/wpiformat/cpplint.py b/wpiformat/wpiformat/cpplint.py index c2cc48b9..508c22d9 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/test/test_virtualspecifier.py b/wpiformat/wpiformat/test/test_virtualspecifier.py new file mode 100644 index 00000000..02d772d9 --- /dev/null +++ b/wpiformat/wpiformat/test/test_virtualspecifier.py @@ -0,0 +1,94 @@ +import os + +from .tasktest import * +from wpiformat.virtualspecifier import VirtualSpecifier + + +def test_virtualspecifier(): + test = TaskTest(VirtualSpecifier()) + + # Redundant virtual specifier on function + test.add_input("./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) + test.add_output( + "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 + test.add_input("./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) + test.add_output( + "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 + test.add_input("./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) + test.add_output( + "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 + test.add_input("./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) + test.add_output( + "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 + test.add_input("./PWM.h", + "class PWM : public SendableBase {" + os.linesep + \ + " public:" + os.linesep + \ + " explicit PWM(int channel);" + os.linesep + \ + " virtual ~PWM();" + os.linesep + \ + "};" + os.linesep) + test.add_output( + "class PWM : public SendableBase {" + os.linesep + \ + " public:" + os.linesep + \ + " explicit PWM(int channel);" + os.linesep + \ + " virtual ~PWM();" + os.linesep + \ + "};" + os.linesep, False, True) + + test.run(OutputType.FILE) diff --git a/wpiformat/wpiformat/virtualspecifier.py b/wpiformat/wpiformat/virtualspecifier.py new file mode 100644 index 00000000..aa3ad9d8 --- /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 = r"(?Pvirtual\s+)?" + return_type = r"(?P[\w,\*&]+\s+)" + func_args = r"(?P\([^\)]*\)(\s*const)?)" + specifiers = r"(?P[^;{]*)?" + + # Construct regexes for function signatures + regexes = [] + regexes.append( + regex.compile(virtual_spec + r"(?P" + return_type + + r"(?P\w+\s*)" + func_args + ")" + + specifiers)) + regexes.append( + regex.compile(virtual_spec + r"(?P" + + r"(?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(r"\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)