Skip to content

Commit 3de80ed

Browse files
calcmogulcalcmogul
committed
Added task for removing redundant "virtual" specifier instances
1 parent 9d06be3 commit 3de80ed

File tree

4 files changed

+268
-95
lines changed

4 files changed

+268
-95
lines changed

Diff for: ‎wpiformat/wpiformat/__init__.py

+2
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
from wpiformat.task import Task
2626
from wpiformat.usingdeclaration import UsingDeclaration
2727
from wpiformat.usingnamespacestd import UsingNamespaceStd
28+
from wpiformat.virtualspecifier import VirtualSpecifier
2829
from wpiformat.whitespace import Whitespace
2930

3031

@@ -492,6 +493,7 @@ def main():
492493
IncludeOrder(),
493494
UsingDeclaration(),
494495
UsingNamespaceStd(),
496+
VirtualSpecifier(),
495497
Whitespace(),
496498
ClangFormat(args.clang_version),
497499
Jni(), # Fixes clang-format formatting

Diff for: ‎wpiformat/wpiformat/cpplint.py

-95
Original file line numberDiff line numberDiff line change
@@ -3447,99 +3447,6 @@ def CheckMakePairUsesDeduction(filename, clean_lines, linenum, error):
34473447
' OR use pair directly OR if appropriate, construct a pair directly')
34483448

34493449

3450-
def CheckRedundantVirtual(filename, clean_lines, linenum, error):
3451-
"""Check if line contains a redundant "virtual" function-specifier.
3452-
3453-
Args:
3454-
filename: The name of the current file.
3455-
clean_lines: A CleansedLines instance containing the file.
3456-
linenum: The number of the line to check.
3457-
error: The function to call with any errors found.
3458-
"""
3459-
# Look for "virtual" on current line.
3460-
line = clean_lines.elided[linenum]
3461-
virtual = Match(r'^(.*)(\bvirtual\b)(.*)$', line)
3462-
if not virtual: return
3463-
3464-
# Ignore "virtual" keywords that are near access-specifiers. These
3465-
# are only used in class base-specifier and do not apply to member
3466-
# functions.
3467-
if (Search(r'\b(public|protected|private)\s+$', virtual.group(1)) or
3468-
Match(r'^\s+(public|protected|private)\b', virtual.group(3))):
3469-
return
3470-
3471-
# Ignore the "virtual" keyword from virtual base classes. Usually
3472-
# there is a column on the same line in these cases (virtual base
3473-
# classes are rare in google3 because multiple inheritance is rare).
3474-
if Match(r'^.*[^:]:[^:].*$', line): return
3475-
3476-
# Look for the next opening parenthesis. This is the start of the
3477-
# parameter list (possibly on the next line shortly after virtual).
3478-
# TODO(unknown): doesn't work if there are virtual functions with
3479-
# decltype() or other things that use parentheses, but csearch suggests
3480-
# that this is rare.
3481-
end_col = -1
3482-
end_line = -1
3483-
start_col = len(virtual.group(2))
3484-
for start_line in range(linenum, min(linenum + 3, clean_lines.NumLines())):
3485-
line = clean_lines.elided[start_line][start_col:]
3486-
parameter_list = Match(r'^([^(]*)\(', line)
3487-
if parameter_list:
3488-
# Match parentheses to find the end of the parameter list
3489-
(_, end_line, end_col) = CloseExpression(
3490-
clean_lines, start_line, start_col + len(parameter_list.group(1)))
3491-
break
3492-
start_col = 0
3493-
3494-
if end_col < 0:
3495-
return # Couldn't find end of parameter list, give up
3496-
3497-
# Look for "override" or "final" after the parameter list
3498-
# (possibly on the next few lines).
3499-
for i in range(end_line, min(end_line + 3, clean_lines.NumLines())):
3500-
line = clean_lines.elided[i][end_col:]
3501-
match = Search(r'\b(override|final)\b', line)
3502-
if match:
3503-
error(filename, linenum, 'readability/inheritance', 4,
3504-
('"virtual" is redundant since function is '
3505-
'already declared as "%s"' % match.group(1)))
3506-
3507-
# Set end_col to check whole lines after we are done with the
3508-
# first line.
3509-
end_col = 0
3510-
if Search(r'[^\w]\s*$', line):
3511-
break
3512-
3513-
3514-
def CheckRedundantOverrideOrFinal(filename, clean_lines, linenum, error):
3515-
"""Check if line contains a redundant "override" or "final" virt-specifier.
3516-
3517-
Args:
3518-
filename: The name of the current file.
3519-
clean_lines: A CleansedLines instance containing the file.
3520-
linenum: The number of the line to check.
3521-
error: The function to call with any errors found.
3522-
"""
3523-
# Look for closing parenthesis nearby. We need one to confirm where
3524-
# the declarator ends and where the virt-specifier starts to avoid
3525-
# false positives.
3526-
line = clean_lines.elided[linenum]
3527-
declarator_end = line.rfind(')')
3528-
if declarator_end >= 0:
3529-
fragment = line[declarator_end:]
3530-
else:
3531-
if linenum > 1 and clean_lines.elided[linenum - 1].rfind(')') >= 0:
3532-
fragment = line
3533-
else:
3534-
return
3535-
3536-
# Check that at most one of "override" or "final" is present, not both
3537-
if Search(r'\boverride\b', fragment) and Search(r'\bfinal\b', fragment):
3538-
error(filename, linenum, 'readability/inheritance', 4,
3539-
('"override" is redundant since function is '
3540-
'already declared as "final"'))
3541-
3542-
35433450
# Returns true if we are at a new block, and it is directly
35443451
# inside of a namespace.
35453452
def IsBlockInNameSpace(nesting_state, is_forward_declaration):
@@ -3591,8 +3498,6 @@ def ProcessLine(filename, is_header, clean_lines, line,
35913498
nesting_state, error)
35923499
CheckInvalidIncrement(filename, clean_lines, line, error)
35933500
CheckMakePairUsesDeduction(filename, clean_lines, line, error)
3594-
CheckRedundantVirtual(filename, clean_lines, line, error)
3595-
CheckRedundantOverrideOrFinal(filename, clean_lines, line, error)
35963501

35973502

35983503
def ProcessFileData(filename, is_header, lines, error):

Diff for: ‎wpiformat/wpiformat/test/test_virtualspecifier.py

+179
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,179 @@
1+
import os
2+
3+
from .tasktest import *
4+
from wpiformat.virtualspecifier import VirtualSpecifier
5+
6+
7+
def test_virtualspecifier():
8+
test = TaskTest(VirtualSpecifier())
9+
10+
# Redundant virtual specifier on function
11+
test.add_input(
12+
"./PWM.h",
13+
"class PWM : public SendableBase {"
14+
+ os.linesep
15+
+ " public:"
16+
+ os.linesep
17+
+ " explicit PWM(int channel);"
18+
+ os.linesep
19+
+ " ~PWM() override;"
20+
+ os.linesep
21+
+ os.linesep
22+
+ " protected:"
23+
+ os.linesep
24+
+ " virtual void InitSendable(SendableBuilder& builder) override;"
25+
+ os.linesep
26+
+ "};"
27+
+ os.linesep,
28+
)
29+
test.add_output(
30+
"class PWM : public SendableBase {"
31+
+ os.linesep
32+
+ " public:"
33+
+ os.linesep
34+
+ " explicit PWM(int channel);"
35+
+ os.linesep
36+
+ " ~PWM() override;"
37+
+ os.linesep
38+
+ os.linesep
39+
+ " protected:"
40+
+ os.linesep
41+
+ " void InitSendable(SendableBuilder& builder) override;"
42+
+ os.linesep
43+
+ "};"
44+
+ os.linesep,
45+
True,
46+
True,
47+
)
48+
49+
# Redundant virtual specifier on const function
50+
test.add_input(
51+
"./PIDController.h",
52+
"class PIDController : public PIDInterface {"
53+
+ os.linesep
54+
+ " public:"
55+
+ os.linesep
56+
+ " virtual double GetP() const override;"
57+
+ os.linesep
58+
+ " virtual double GetI() const override;"
59+
+ os.linesep
60+
+ " virtual double GetD() const override;"
61+
+ os.linesep
62+
+ "};"
63+
+ os.linesep,
64+
)
65+
test.add_output(
66+
"class PIDController : public PIDInterface {"
67+
+ os.linesep
68+
+ " public:"
69+
+ os.linesep
70+
+ " double GetP() const override;"
71+
+ os.linesep
72+
+ " double GetI() const override;"
73+
+ os.linesep
74+
+ " double GetD() const override;"
75+
+ os.linesep
76+
+ "};"
77+
+ os.linesep,
78+
True,
79+
True,
80+
)
81+
82+
# Redundant final specifier on const function
83+
test.add_input(
84+
"./PIDController.h",
85+
"class PIDController : public PIDInterface {"
86+
+ os.linesep
87+
+ " public:"
88+
+ os.linesep
89+
+ " double GetP() const override;"
90+
+ os.linesep
91+
+ " double GetI() const final override;"
92+
+ os.linesep
93+
+ " double GetD() const override final;"
94+
+ os.linesep
95+
+ "};"
96+
+ os.linesep,
97+
)
98+
test.add_output(
99+
"class PIDController : public PIDInterface {"
100+
+ os.linesep
101+
+ " public:"
102+
+ os.linesep
103+
+ " double GetP() const override;"
104+
+ os.linesep
105+
+ " double GetI() const final;"
106+
+ os.linesep
107+
+ " double GetD() const final;"
108+
+ os.linesep
109+
+ "};"
110+
+ os.linesep,
111+
True,
112+
True,
113+
)
114+
115+
# Redundant virtual specifier on destructor
116+
test.add_input(
117+
"./PWM.h",
118+
"class PWM : public SendableBase {"
119+
+ os.linesep
120+
+ " public:"
121+
+ os.linesep
122+
+ " explicit PWM(int channel);"
123+
+ os.linesep
124+
+ " virtual ~PWM() override;"
125+
+ os.linesep
126+
+ os.linesep
127+
+ " virtual void SetRaw(uint16_t value);"
128+
+ os.linesep
129+
+ "};"
130+
+ os.linesep,
131+
)
132+
test.add_output(
133+
"class PWM : public SendableBase {"
134+
+ os.linesep
135+
+ " public:"
136+
+ os.linesep
137+
+ " explicit PWM(int channel);"
138+
+ os.linesep
139+
+ " ~PWM() override;"
140+
+ os.linesep
141+
+ os.linesep
142+
+ " virtual void SetRaw(uint16_t value);"
143+
+ os.linesep
144+
+ "};"
145+
+ os.linesep,
146+
True,
147+
True,
148+
)
149+
150+
# Idempotence with virtual specifier on destructor
151+
test.add_input(
152+
"./PWM.h",
153+
"class PWM : public SendableBase {"
154+
+ os.linesep
155+
+ " public:"
156+
+ os.linesep
157+
+ " explicit PWM(int channel);"
158+
+ os.linesep
159+
+ " virtual ~PWM();"
160+
+ os.linesep
161+
+ "};"
162+
+ os.linesep,
163+
)
164+
test.add_output(
165+
"class PWM : public SendableBase {"
166+
+ os.linesep
167+
+ " public:"
168+
+ os.linesep
169+
+ " explicit PWM(int channel);"
170+
+ os.linesep
171+
+ " virtual ~PWM();"
172+
+ os.linesep
173+
+ "};"
174+
+ os.linesep,
175+
False,
176+
True,
177+
)
178+
179+
test.run(OutputType.FILE)

Diff for: ‎wpiformat/wpiformat/virtualspecifier.py

+87
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
"""This task removes redundant "virtual" specifier instances.
2+
3+
When the "override" specifier is specified in a C++ function signature, the
4+
"virtual" specifier is redundant because "override" implies "virtual".
5+
"""
6+
7+
import regex
8+
9+
from wpiformat.task import Task
10+
11+
12+
class VirtualSpecifier(Task):
13+
def should_process_file(self, config_file, name):
14+
return config_file.is_cpp_file(name)
15+
16+
def run_pipeline(self, config_file, name, lines):
17+
linesep = Task.get_linesep(lines)
18+
19+
file_changed = False
20+
output = ""
21+
22+
# Function signature parts
23+
virtual_spec = r"(?P<virtual>virtual\s+)?"
24+
return_type = r"(?P<return_type>[\w,\*&]+\s+)"
25+
func_args = r"(?P<func_args>\([^\)]*\)(\s*const)?)"
26+
specifiers = r"(?P<specifiers>[^;{]*)?"
27+
28+
# Construct regexes for function signatures
29+
regexes = []
30+
regexes.append(
31+
regex.compile(
32+
virtual_spec
33+
+ r"(?P<func_sig>"
34+
+ return_type
35+
+ r"(?P<func_name>\w+\s*)"
36+
+ func_args
37+
+ ")"
38+
+ specifiers
39+
)
40+
)
41+
regexes.append(
42+
regex.compile(
43+
virtual_spec
44+
+ r"(?P<func_sig>"
45+
+ r"(?P<func_name>[\w~]+\s*)"
46+
+ func_args
47+
+ ")"
48+
+ specifiers
49+
)
50+
)
51+
52+
for rgx in regexes:
53+
pos = 0
54+
for match in rgx.finditer(lines):
55+
# Append lines before match
56+
output += lines[pos : match.start()]
57+
58+
# Append virtual specifier if it's not redundant
59+
if "final" not in match.group(
60+
"specifiers"
61+
) and "override" not in match.group("specifiers"):
62+
if match.group("virtual"):
63+
output += match.group("virtual")
64+
else:
65+
file_changed = True
66+
output += match.group("func_sig")
67+
68+
# Strip redundant specifiers
69+
specifiers_in = match.group("specifiers")
70+
specifiers_out = specifiers_in
71+
if "final" in specifiers_out:
72+
specifiers_out = regex.sub(r"\s+override", "", specifiers_out)
73+
if specifiers_in != specifiers_out:
74+
file_changed = True
75+
output += specifiers_out
76+
77+
pos = match.end()
78+
79+
# Append leftover lines in file
80+
if pos < len(lines):
81+
output += lines[pos:]
82+
83+
# Reset line buffer for next regex
84+
lines = output
85+
output = ""
86+
87+
return (lines, file_changed, True)

0 commit comments

Comments
 (0)