Skip to content

Commit

Permalink
Added task for removing redundant "virtual" specifier instances
Browse files Browse the repository at this point in the history
  • Loading branch information
calcmogul committed May 6, 2024
1 parent 6694a03 commit 2880131
Show file tree
Hide file tree
Showing 4 changed files with 268 additions and 95 deletions.
2 changes: 2 additions & 0 deletions wpiformat/wpiformat/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand Down Expand Up @@ -526,6 +527,7 @@ def main():
IncludeOrder(),
UsingDeclaration(),
UsingNamespaceStd(),
VirtualSpecifier(),
Whitespace(),
ClangFormat(args.clang_version),
Jni(), # Fixes clang-format formatting
Expand Down
95 changes: 0 additions & 95 deletions wpiformat/wpiformat/cpplint.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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):
Expand Down
179 changes: 179 additions & 0 deletions wpiformat/wpiformat/test/test_virtualspecifier.py
Original file line number Diff line number Diff line change
@@ -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)
87 changes: 87 additions & 0 deletions wpiformat/wpiformat/virtualspecifier.py
Original file line number Diff line number Diff line change
@@ -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)

0 comments on commit 2880131

Please sign in to comment.