Merge pull request #1245 from geoffw0/classesmanyfields

CPP: Fix performance issues in ClassesWithManyFields.ql
2025-12-16 16:53:25 +01:00 · 2019-05-01 13:58:28 +02:00
parent d4e1bae5d5 efa3c77db5
commit 490dd0e8c0
3 changed files with 149 additions and 63 deletions
--- a/Opportunities/ClassesWithManyFields.ql
+++ b/Opportunities/ClassesWithManyFields.ql
@@ -12,6 +12,9 @@

 import cpp

+/**
+ * Gets a string describing the kind of a `Class`.
+ */
 string kindstr(Class c) {
  exists(int kind | usertypes(unresolveElement(c), _, kind) |
    kind = 1 and result = "Struct"
@@ -22,76 +25,129 @@ string kindstr(Class c) {
  )
 }

+/**
+ * Holds if the arguments correspond to information about a `VariableDeclarationEntry`.
+ */
 predicate vdeInfo(VariableDeclarationEntry vde, Class c, File f, int line) {
  c = vde.getVariable().getDeclaringType() and
  f = vde.getLocation().getFile() and
  line = vde.getLocation().getStartLine()
 }

-predicate previousVde(VariableDeclarationEntry previous, VariableDeclarationEntry vde) {
-  exists(Class c, File f, int line | vdeInfo(vde, c, f, line) |
-    vdeInfo(previous, c, f, line - 3)
-    or
-    vdeInfo(previous, c, f, line - 2)
-    or
-    vdeInfo(previous, c, f, line - 1)
-    or
-    vdeInfo(previous, c, f, line) and
-    exists(int prevCol, int vdeCol |
-      prevCol = previous.getLocation().getStartColumn() and
-      vdeCol = vde.getLocation().getStartColumn()
-    |
-      prevCol < vdeCol
-      or
-      prevCol = vdeCol and previous.getName() < vde.getName()
-    )
-  )
+newtype TVariableDeclarationInfo =
+  TVariableDeclarationLine(Class c, File f, int line) { vdeInfo(_, c, f, line) }
+
+/**
+ * A line that contains one or more `VariableDeclarationEntry`s (in the same class).
+ */
+class VariableDeclarationLine extends TVariableDeclarationInfo {
+  Class c;
+
+  File f;
+
+  int line;
+
+  VariableDeclarationLine() {
+    vdeInfo(_, c, f, line) and
+    this = TVariableDeclarationLine(c, f, line)
+  }
+
+  /**
+   * Gets the class associated with this `VariableDeclarationLine`.
+   */
+  Class getClass() { result = c }
+
+  /**
+   * Gets the line of this `VariableDeclarationLine`.
+   */
+  int getLine() { result = line }
+
+  /**
+   * Gets a `VariableDeclarationEntry` on this line.
+   */
+  VariableDeclarationEntry getAVDE() { vdeInfo(result, c, f, line) }
+
+  /**
+   * Gets the start column of the first `VariableDeclarationEntry` on this line.
+   */
+  int getStartColumn() { result = min(getAVDE().getLocation().getStartColumn()) }
+
+  /**
+   * Gets the end column of the last `VariableDeclarationEntry` on this line.
+   */
+  int getEndColumn() { result = max(getAVDE().getLocation().getEndColumn()) }
+
+  /**
+   * Gets the rank of this `VariableDeclarationLine` in its file and class
+   * (that is, the first is 0, the second is 1 and so on).
+   */
+  private int getRank() {
+    line = rank[result](VariableDeclarationLine vdl, int l |
+        vdl = TVariableDeclarationLine(c, f, l)
+      |
+        l
+      )
+  }
+
+  /**
+   * Gets the `VariableDeclarationLine` following this one, if any.
+   */
+  VariableDeclarationLine getNext() {
+    result = TVariableDeclarationLine(c, f, _) and
+    result.getRank() = getRank() + 1
+  }
+
+  /**
+   * Gets the `VariableDeclarationLine` following this one, if it is nearby.
+   */
+  VariableDeclarationLine getProximateNext() {
+    result = getNext() and
+    result.getLine() <= this.getLine() + 3
+  }
+
+  string toString() { result = "VariableDeclarationLine" }
 }

-predicate masterVde(VariableDeclarationEntry master, VariableDeclarationEntry vde) {
-  not previousVde(_, vde) and master = vde
-  or
-  exists(VariableDeclarationEntry previous |
-    previousVde(previous, vde) and masterVde(master, previous)
-  )
-}
+/**
+ * A group of `VariableDeclarationEntry`s in the same class that are approximately
+ * contiguous.
+ */
+class VariableDeclarationGroup extends VariableDeclarationLine {
+  VariableDeclarationLine end;

-class VariableDeclarationGroup extends ElementBase {
  VariableDeclarationGroup() {
-    this instanceof VariableDeclarationEntry and
-    not previousVde(_, this)
+    // there is no `VariableDeclarationLine` within three lines previously
+    not any(VariableDeclarationLine prev).getProximateNext() = this and
+    // `end` is the last transitively proximate line
+    end = getProximateNext*() and
+    not exists(end.getProximateNext())
  }

-  Class getClass() { vdeInfo(this, result, _, _) }
-
-  // pragma[noopt] since otherwise the two locationInfo relations get join-ordered
-  // after each other
-  pragma[noopt]
  predicate hasLocationInfo(string path, int startline, int startcol, int endline, int endcol) {
-    exists(VariableDeclarationEntry last, Location lstart, Location lend |
-      masterVde(this, last) and
-      this instanceof VariableDeclarationGroup and
-      not previousVde(last, _) and
-      exists(VariableDeclarationEntry vde |
-        vde = this and vde instanceof VariableDeclarationEntry and vde.getLocation() = lstart
-      ) and
-      last.getLocation() = lend and
-      lstart.hasLocationInfo(path, startline, startcol, _, _) and
-      lend.hasLocationInfo(path, _, _, endline, endcol)
-    )
+    path = f.getAbsolutePath() and
+    startline = getLine() and
+    startcol = getStartColumn() and
+    endline = end.getLine() and
+    endcol = end.getEndColumn()
  }

-  string describeGroup() {
-    if previousVde(this, _)
-    then
-      result = "group of " +
-          strictcount(string name |
-            exists(VariableDeclarationEntry vde |
-              masterVde(this, vde) and
-              name = vde.getName()
-            )
-          ) + " fields here"
-    else result = "declaration of " + this.(VariableDeclarationEntry).getVariable().getName()
+  /**
+   * Gets the number of uniquely named `VariableDeclarationEntry`s in this group.
+   */
+  int getCount() {
+    result = count(VariableDeclarationLine l |
+        l = getProximateNext*()
+      |
+        l.getAVDE().getVariable().getName()
+      )
+  }
+
+  override string toString() {
+    getCount() = 1 and
+    result = "declaration of " + getAVDE().getVariable().getName()
+    or
+    getCount() > 1 and
+    result = "group of " + getCount() + " fields here"
  }
 }

@@ -129,4 +185,4 @@ where
  if c.hasOneVariableGroup() then suffix = "" else suffix = " - see $@"
 select c,
  kindstr(c) + " " + c.getName() + " has " + n +
-    " fields; we suggest refactoring to 15 fields or fewer" + suffix + ".", vdg, vdg.describeGroup()
+    " fields; we suggest refactoring to 15 fields or fewer" + suffix + ".", vdg, vdg.toString()
--- a/cpp/ql/test/query-tests/Architecture/Refactoring
+++ b/cpp/ql/test/query-tests/Architecture/Refactoring
@@ -56,3 +56,29 @@ struct MyParticle {
  class texture *tex;
  float u1, v1, u2, v2;
 };
+
+struct MyAlphaClass1 {
+  int a1, b1, c1, d1, e1, f1, g1, h1, i1, j1;
+  int k1, l1, m1, n1, o1, p1, q1, r1, s1, t1;
+  int u1, v1, w1, x1, y1, z1;
+
+  // ...
+  // ...
+  // ...
+
+  int a2, b2, c2, d2, e2, f2, g2, h2, i2, j2;
+  int k2, l2, m2, n2, o2, p2, q2, r2, s2, t2;
+  int u2, v2, w2, x2, y2, z2;
+};
+
+struct MyAlphaClass2 {
+  int x;
+
+  // ...
+  // ...
+  // ...
+
+  int a1, b1, c1, d1, e1, f1, g1, h1, i1, j1;
+  int k1, l1, m1, n1, o1, p1, q1, r1, s1, t1;
+  int u1, v1, w1, x1, y1, z1;
+};
--- a/Opportunities/ClassesWithManyFields/cwmf.expected
+++ b/Opportunities/ClassesWithManyFields/cwmf.expected
@@ -1,7 +1,11 @@
-| 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 |
-| 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 |
-| 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 |
-| 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 |
-| 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 |
-| 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 |
-| 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 |
+| 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 |
+| 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 |
+| 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 |
+| 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 |
+| 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 |
+| cwmf.cpp:60:8:60:20 | MyAlphaClass1 | Struct MyAlphaClass1 has 52 fields; we suggest refactoring to 15 fields or fewer - see $@. | cwmf.cpp:61:7:63:28 | group of 26 fields here | group of 26 fields here |
+| cwmf.cpp:60:8:60:20 | MyAlphaClass1 | Struct MyAlphaClass1 has 52 fields; we suggest refactoring to 15 fields or fewer - see $@. | cwmf.cpp:69:7:71:28 | group of 26 fields here | group of 26 fields here |
+| cwmf.cpp:74:8:74:20 | MyAlphaClass2 | Struct MyAlphaClass2 has 27 fields; we suggest refactoring to 15 fields or fewer - see $@. | cwmf.cpp:75:7:75:7 | declaration of x | declaration of x |
+| cwmf.cpp:74:8:74:20 | MyAlphaClass2 | Struct MyAlphaClass2 has 27 fields; we suggest refactoring to 15 fields or fewer - see $@. | cwmf.cpp:81:7:83:28 | group of 26 fields here | group of 26 fields here |
+| 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 |
+| 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 |