Skip to content

Commit a9984e9

Browse files
committed
C++: Implement Pos and Spec as int, not newtype
This change gives a slight performance improvement and makes the QL code shorter. It introduces some magic numbers in the code, but those are confined to the `Pos` and `Spec` classes. We get a speed-up because the evaluator has built-in support for integer literals in the `OUTPUT` of `JOIN` operations, whereas `newtype`s have to be explicitly joined on. As a result, a predicate like `CFG::straightLineSparse#ffff` drops from 262 pipeline nodes to 242. I measured performance on https://github.com/jluttine/suitesparse, which is one of the projects that had the biggest slowdown when enabling the QL CFG on lgtm.com. I took two measurements before this change and two after. The `CFG.qll` stage took 117s and 112s before, and it took 106s and 107s after.
1 parent b142113 commit a9984e9

File tree

1 file changed

+14
-51
lines changed
  • cpp/ql/src/semmle/code/cpp/controlflow/internal

1 file changed

+14
-51
lines changed

cpp/ql/src/semmle/code/cpp/controlflow/internal/CFG.qll

Lines changed: 14 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -173,49 +173,36 @@ predicate excludeNode(Node n) {
173173
excludeNode(n.getParentNode())
174174
}
175175

176-
private newtype TPos =
177-
PosBefore() or
178-
PosAt() or
179-
PosAfter() or
180-
PosBeforeDestructors() or
181-
PosAfterDestructors()
182-
183-
/** A `Pos` without a `bindingset` requirement on the constructor. */
184-
private class AnyPos extends TPos {
185-
string toString() { result = "Pos" }
186-
}
187-
188176
/**
189177
* A constant that indicates the type of sub-node in a pair of `(Node, Pos)`.
190178
* See the comment block at the top of this file.
191179
*/
192-
private class Pos extends AnyPos {
193-
// This is to make sure we get compile errors in code that forgets to restrict a `Pos`.
180+
private class Pos extends int {
194181
bindingset[this]
195182
Pos() { any() }
196183

197184
/** Holds if this is the position just _before_ the associated `Node`. */
198-
predicate isBefore() { this = PosBefore() }
185+
predicate isBefore() { this = 0 }
199186

200187
/** Holds if `(n, this)` is the sub-node that represents `n` itself. */
201-
predicate isAt() { this = PosAt() }
188+
predicate isAt() { this = 1 }
202189

203190
/** Holds if this is the position just _after_ the associated `Node`. */
204-
predicate isAfter() { this = PosAfter() }
191+
predicate isAfter() { this = 2 }
205192

206193
/**
207194
* Holds if `(n, this)` is the virtual sub-node that comes just _before_ any
208195
* implicit destructor calls following `n`. The node `n` will be some node
209196
* that may be followed by local variables going out of scope.
210197
*/
211-
predicate isBeforeDestructors() { this = PosBeforeDestructors() }
198+
predicate isBeforeDestructors() { this = 3 }
212199

213200
/**
214201
* Holds if `(n, this)` is the virtual sub-node that comes just _after_ any
215202
* implicit destructor calls following `n`. The node `n` will be some node
216203
* that may be followed by local variables going out of scope.
217204
*/
218-
predicate isAfterDestructors() { this = PosAfterDestructors() }
205+
predicate isAfterDestructors() { this = 4 }
219206

220207
pragma[inline]
221208
predicate nodeBefore(Node n, Node nEq) { this.isBefore() and n = nEq }
@@ -489,51 +476,25 @@ private Node getLastControlOrderChild(Node n) {
489476
result = getControlOrderChildDense(n, max(int i | exists(getControlOrderChildDense(n, i))))
490477
}
491478

492-
private newtype TSpec =
493-
SpecPos(AnyPos p) or
494-
SpecAround() or
495-
SpecAroundDestructors() or
496-
SpecBarrier()
497-
498-
/** A `Spec` without a `bindingset` requirement on the constructor. */
499-
private class AnySpec extends TSpec {
500-
string toString() { result = "Spec" }
501-
}
502-
503479
/**
504480
* A constant that represents two positions: one position for when it's used as
505481
* the _source_ of a sub-edge, and another position for when it's used as the
506482
* _target_. Values include all the values of `Pos`, which resolve to
507483
* themselves as both source and target, as well as two _around_ values and a
508484
* _barrier_ value.
509485
*/
510-
private class Spec extends AnySpec {
486+
private class Spec extends Pos {
511487
bindingset[this]
512488
Spec() { any() }
513489

514-
/** See Pos.isBefore. */
515-
predicate isBefore() { this = SpecPos(PosBefore()) }
516-
517-
/** See Pos.isAt. */
518-
predicate isAt() { this = SpecPos(PosAt()) }
519-
520-
/** See Pos.isAfter. */
521-
predicate isAfter() { this = SpecPos(PosAfter()) }
522-
523-
/** See Pos.isBeforeDestructors. */
524-
predicate isBeforeDestructors() { this = SpecPos(PosBeforeDestructors()) }
525-
526-
/** See Pos.isAfterDestructors. */
527-
predicate isAfterDestructors() { this = SpecPos(PosAfterDestructors()) }
528-
529490
/**
530491
* Holds if this spec, when used on a node `n` between `(n1, p1)` and
531492
* `(n2, p2)`, should add the following sub-edges.
532493
*
533494
* (n1, p1) ----> before(n)
534495
* after(n) ----> (n2, p2)
535496
*/
536-
predicate isAround() { this = SpecAround() }
497+
predicate isAround() { this = 5 }
537498

538499
/**
539500
* Holds if this spec, when used on a node `n` between `(n1, p1)` and
@@ -542,16 +503,17 @@ private class Spec extends AnySpec {
542503
* (n1, p1) ----> beforeDestructors(n)
543504
* afterDestructors(n) ----> (n2, p2)
544505
*/
545-
predicate isAroundDestructors() { this = SpecAroundDestructors() }
506+
predicate isAroundDestructors() { this = 6 }
546507

547508
/**
548509
* Holds if this node is a _barrier_. A barrier resolves to no positions and
549510
* can be inserted between nodes that should have no sub-edges between them.
550511
*/
551-
predicate isBarrier() { this = SpecBarrier() }
512+
predicate isBarrier() { this = 7 }
552513

553514
Pos getSourcePos() {
554-
this = SpecPos(result)
515+
this = [0..4] and
516+
result = this
555517
or
556518
this.isAround() and
557519
result.isAfter()
@@ -561,7 +523,8 @@ private class Spec extends AnySpec {
561523
}
562524

563525
Pos getTargetPos() {
564-
this = SpecPos(result)
526+
this = [0..4] and
527+
result = this
565528
or
566529
this.isAround() and
567530
result.isBefore()

0 commit comments

Comments
 (0)