Added task for removing redundant "virtual" specifier instances

wpiformat/wpiformat/__init__.py

@@ -26,6 +26,7 @@
 from wpiformat.task import BatchTask, PipelineTask, StandaloneTask, Task
 from wpiformat.usingdeclaration import UsingDeclaration
 from wpiformat.usingnamespacestd import UsingNamespaceStd
+from wpiformat.virtualspecifier import VirtualSpecifier
 from wpiformat.whitespace import Whitespace
 
 
@@ -526,6 +527,7 @@ def main():
             IncludeOrder(),
             UsingDeclaration(),
             UsingNamespaceStd(),
+            VirtualSpecifier(),
             Whitespace(),
             ClangFormat(args.clang_version),
             Jni(),  # Fixes clang-format formatting

wpiformat/wpiformat/cpplint.py

@@ -3451,99 +3451,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):
@@ -3595,8 +3502,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):

wpiformat/wpiformat/test/test_virtualspecifier.py

@@ -0,0 +1,179 @@
+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)

wpiformat/wpiformat/virtualspecifier.py

@@ -0,0 +1,87 @@
+"""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"(?P<virtual>virtual\s+)?"
+        return_type = r"(?P<return_type>[\w,\*&]+\s+)"
+        func_args = r"(?P<func_args>\([^\)]*\)(\s*const)?)"
+        specifiers = r"(?P<specifiers>[^;{]*)?"
+
+        # Construct regexes for function signatures
+        regexes = []
+        regexes.append(
+            regex.compile(
+                virtual_spec
+                + r"(?P<func_sig>"
+                + return_type
+                + r"(?P<func_name>\w+\s*)"
+                + func_args
+                + ")"
+                + specifiers
+            )
+        )
+        regexes.append(
+            regex.compile(
+                virtual_spec
+                + r"(?P<func_sig>"
+                + r"(?P<func_name>[\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)