diff --git a/wpiformat/wpiformat/__init__.py b/wpiformat/wpiformat/__init__.py index 27dc877..3538381 100644 --- a/wpiformat/wpiformat/__init__.py +++ b/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 diff --git a/wpiformat/wpiformat/cpplint.py b/wpiformat/wpiformat/cpplint.py index 53dee70..8b9b5f8 100644 --- a/wpiformat/wpiformat/cpplint.py +++ b/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): diff --git a/wpiformat/wpiformat/test/test_virtualspecifier.py b/wpiformat/wpiformat/test/test_virtualspecifier.py new file mode 100644 index 0000000..56913bc --- /dev/null +++ b/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) diff --git a/wpiformat/wpiformat/virtualspecifier.py b/wpiformat/wpiformat/virtualspecifier.py new file mode 100644 index 0000000..83baab3 --- /dev/null +++ b/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"(?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)