Skip to content

Commit 89c26ca

Browse files
geoffw0geoffw0
committed
CPP: Rewrite the VDE grouping in ClassesWithManyField.ql to be more performant (and modern).
1 parent 0cc4b23 commit 89c26ca

File tree

2 files changed

+120
-57
lines changed

2 files changed

+120
-57
lines changed

‎cpp/ql/src/Architecture/Refactoring Opportunities/ClassesWithManyFields.ql‎

Lines changed: 113 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -33,70 +33,133 @@ predicate vdeInfo(VariableDeclarationEntry vde, Class c, File f, int line)
3333
line = vde.getLocation().getStartLine()
3434
}
3535

36+
newtype TVariableDeclarationInfo =
37+
TVariableDeclarationLine(Class c, File f, int line) {
38+
vdeInfo(_, c, f, line)
39+
}
40+
3641
/**
37-
* Holds if `previous` describes a `VariableDeclarationEntry` occurring soon before
38-
* `vde` (this may have many results).
42+
* A line that contains one or more `VariableDeclarationEntry`s (in the same class).
3943
*/
40-
predicate previousVde(VariableDeclarationEntry previous, VariableDeclarationEntry vde)
44+
class VariableDeclarationLine extends TVariableDeclarationInfo
4145
{
42-
exists(Class c, File f, int line | vdeInfo(vde, c, f, line) |
43-
vdeInfo(previous, c, f, line - 3) or
44-
vdeInfo(previous, c, f, line - 2) or
45-
vdeInfo(previous, c, f, line - 1) or
46-
(vdeInfo(previous, c, f, line) and exists(int prevCol, int vdeCol |
47-
prevCol = previous.getLocation().getStartColumn() and vdeCol = vde.getLocation().getStartColumn() |
48-
prevCol < vdeCol or (prevCol = vdeCol and previous.getName() < vde.getName())
49-
))
50-
)
46+
Class c;
47+
File f;
48+
int line;
49+
50+
VariableDeclarationLine() {
51+
vdeInfo(_, c, f, line) and
52+
this = TVariableDeclarationLine(c, f, line)
53+
}
54+
55+
/**
56+
* Gets the class associated with this `VariableDeclarationLine`.
57+
*/
58+
Class getClass() {
59+
result = c
60+
}
61+
62+
/**
63+
* Gets the line of this `VariableDeclarationLine`.
64+
*/
65+
int getLine() {
66+
result = line
67+
}
68+
69+
/**
70+
* Gets a `VariableDeclarationEntry` on this line.
71+
*/
72+
VariableDeclarationEntry getAVDE()
73+
{
74+
vdeInfo(result, c, f, line)
75+
}
76+
77+
/**
78+
* Gets the start column of the first `VariableDeclarationEntry` on this line.
79+
*/
80+
int getStartColumn() {
81+
result = min(getAVDE().getLocation().getStartColumn())
82+
}
83+
84+
/**
85+
* Gets the end column of the last `VariableDeclarationEntry` on this line.
86+
*/
87+
int getEndColumn() {
88+
result = max(getAVDE().getLocation().getEndColumn())
89+
}
90+
91+
/**
92+
* Gets the rank of this `VariableDeclarationLine` in it's file and class
93+
* (that is, the first is 0, the second is 1 and so on).
94+
*/
95+
private int getRank() {
96+
line = rank[result](VariableDeclarationLine vdl, int l |
97+
vdl = TVariableDeclarationLine(c, f, l) |
98+
l
99+
)
100+
}
101+
102+
/**
103+
* Gets the `VariableDeclarationLine` following this one, if any.
104+
*/
105+
VariableDeclarationLine getNext() {
106+
result = TVariableDeclarationLine(c, f, _) and
107+
result.getRank() = getRank() + 1
108+
}
109+
110+
/**
111+
* Gets the `VariableDeclarationLine` following this one, if it is nearby.
112+
*/
113+
VariableDeclarationLine getProximateNext() {
114+
result = getNext() and
115+
result.getLine() <= this.getLine() + 3
116+
}
117+
118+
string toString() {
119+
result = "VariableDeclarationLine"
120+
}
51121
}
52122

53123
/**
54-
* The first `VariableDeclarationEntry` in a group.
124+
* A group of `VariableDeclarationEntry`s in the same class that are approximately
125+
* contiguous.
55126
*/
56-
predicate masterVde(VariableDeclarationEntry master, VariableDeclarationEntry vde)
127+
class VariableDeclarationGroup extends VariableDeclarationLine
57128
{
58-
(not previousVde(_, vde) and master = vde) or
59-
exists(VariableDeclarationEntry previous | previousVde(previous, vde) and masterVde(master, previous))
60-
}
129+
VariableDeclarationLine end;
61130

62-
/**
63-
* A group of `VariableDeclaratinEntry`'s in the same class and in close proximity
64-
* to each other.
65-
*/
66-
class VariableDeclarationGroup extends ElementBase {
67131
VariableDeclarationGroup() {
68-
this instanceof VariableDeclarationEntry and
69-
not previousVde(_, this)
70-
}
71-
Class getClass() {
72-
vdeInfo(this, result, _, _)
132+
// there is no `VariableDeclarationLine` within three lines previously
133+
not any(VariableDeclarationLine prev).getProximateNext() = this and
134+
135+
// `end` is the last transitively proximate line
136+
end = getProximateNext*() and
137+
not exists(end.getProximateNext())
73138
}
74139

75-
// pragma[noopt] since otherwise the two locationInfo relations get join-ordered
76-
// after each other
77-
pragma[noopt]
78140
predicate hasLocationInfo(string path, int startline, int startcol, int endline, int endcol) {
79-
exists(VariableDeclarationEntry last, Location lstart, Location lend |
80-
masterVde(this, last) and
81-
this instanceof VariableDeclarationGroup and
82-
not previousVde(last, _) and
83-
exists(VariableDeclarationEntry vde | vde=this and vde instanceof VariableDeclarationEntry and vde.getLocation() = lstart) and
84-
last.getLocation() = lend and
85-
lstart.hasLocationInfo(path, startline, startcol, _, _) and
86-
lend.hasLocationInfo(path, _, _, endline, endcol)
87-
)
141+
path = f.getAbsolutePath() and
142+
startline = getLine() and
143+
startcol = getStartColumn() and
144+
endline = end.getLine() and
145+
endcol = end.getEndColumn()
88146
}
89147

90-
string describeGroup() {
91-
if previousVde(this, _) then
92-
result = "group of "
93-
+ strictcount(string name
94-
| exists(VariableDeclarationEntry vde
95-
| masterVde(this, vde) and
96-
name = vde.getName()))
97-
+ " fields here"
98-
else
99-
result = "declaration of " + this.(VariableDeclarationEntry).getVariable().getName()
148+
/**
149+
* Gets the number of uniquely named `VariableDeclarationEntry`s in this group.
150+
*/
151+
int getCount() {
152+
result = count(VariableDeclarationLine l | l = getProximateNext*() | l.getAVDE().getVariable().getName())
153+
}
154+
155+
override string toString() {
156+
(
157+
getCount() = 1 and
158+
result = "declaration of " + getAVDE().getVariable().getName()
159+
) or (
160+
getCount() > 1 and
161+
result = "group of " + getCount() + " fields here"
162+
)
100163
}
101164
}
102165

@@ -128,4 +191,4 @@ where n = strictcount(string fieldName
128191
c = vdg.getClass() and
129192
if c.hasOneVariableGroup() then suffix = "" else suffix = " - see $@"
130193
select c, kindstr(c) + " " + c.getName() + " has " + n + " fields; we suggest refactoring to 15 fields or fewer" + suffix + ".",
131-
vdg, vdg.describeGroup()
194+
vdg, vdg.toString()

‎cpp/ql/test/query-tests/Architecture/Refactoring Opportunities/ClassesWithManyFields/cwmf.expected‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
1-
| cwmf.cpp:8:3:9:12 | aa | Struct aa has 20 fields; we suggest refactoring to 15 fields or fewer. | cwmf.cpp:8:3:9:12 | definition of f1 | group of 20 fields here |
2-
| cwmf.cpp:13:3:14:12 | bb | Class bb has 20 fields; we suggest refactoring to 15 fields or fewer. | cwmf.cpp:13:3:14:12 | definition of f1 | group of 20 fields here |
3-
| cwmf.cpp:24:3:25:12 | dd<T> | Template class dd<T> has 20 fields; we suggest refactoring to 15 fields or fewer. | cwmf.cpp:24:3:25:12 | definition of f1 | group of 20 fields here |
4-
| cwmf.cpp:30:3:31:12 | ee<U> | Template class ee<U> has 20 fields; we suggest refactoring to 15 fields or fewer. | cwmf.cpp:30:3:31:12 | definition of f1 | group of 20 fields here |
5-
| cwmf.cpp:41:8:57:22 | MyParticle | Struct MyParticle has 30 fields; we suggest refactoring to 15 fields or fewer. | cwmf.cpp:41:8:57:22 | definition of isActive | group of 30 fields here |
6-
| different_types.h:15:15:33:10 | DifferentTypes2 | Class DifferentTypes2 has 18 fields; we suggest refactoring to 15 fields or fewer. | different_types.h:15:15:33:10 | definition of i1 | group of 18 fields here |
7-
| different_types.h:15:15:33:10 | DifferentTypes2 | Class DifferentTypes2 has 18 fields; we suggest refactoring to 15 fields or fewer. | different_types.h:15:15:33:10 | definition of i1 | group of 18 fields here |
1+
| cwmf.cpp:8:3:9:12 | aa | Struct aa has 20 fields; we suggest refactoring to 15 fields or fewer. | cwmf.cpp:8:3:9:12 | group of 20 fields here | group of 20 fields here |
2+
| cwmf.cpp:13:3:14:12 | bb | Class bb has 20 fields; we suggest refactoring to 15 fields or fewer. | cwmf.cpp:13:3:14:12 | group of 20 fields here | group of 20 fields here |
3+
| cwmf.cpp:24:3:25:12 | dd<T> | Template class dd<T> has 20 fields; we suggest refactoring to 15 fields or fewer. | cwmf.cpp:24:3:25:12 | group of 20 fields here | group of 20 fields here |
4+
| cwmf.cpp:30:3:31:12 | ee<U> | Template class ee<U> has 20 fields; we suggest refactoring to 15 fields or fewer. | cwmf.cpp:30:3:31:12 | group of 20 fields here | group of 20 fields here |
5+
| cwmf.cpp:41:8:57:22 | MyParticle | Struct MyParticle has 30 fields; we suggest refactoring to 15 fields or fewer. | cwmf.cpp:41:8:57:22 | group of 30 fields here | group of 30 fields here |
6+
| different_types.h:15:15:33:10 | DifferentTypes2 | Class DifferentTypes2 has 18 fields; we suggest refactoring to 15 fields or fewer. | different_types.h:15:15:33:10 | group of 18 fields here | group of 18 fields here |
7+
| different_types.h:15:15:33:10 | DifferentTypes2 | Class DifferentTypes2 has 18 fields; we suggest refactoring to 15 fields or fewer. | different_types.h:15:15:33:10 | group of 18 fields here | group of 18 fields here |

0 commit comments

Comments
 (0)