Ruby: Rework getConstantValue implementation#8484
Conversation
649fa28 to
eee4941
Compare
eee4941 to
99ddfb4
Compare
|
This will conflict with #7985, if we decide that's a change we want. It should be easy to resolve the conflicts, but we might want to consider the consequences for predicate names in this PR (e.g. |
nickrolfe
left a comment
There was a problem hiding this comment.
I'm only half-way through reviewing, but what I've seen so far makes sense, and it looks like the test output shows only improvements.
The isT/isTExprNoCfg/isTExpr predicates do seem quite boilerplatey, but they're also reasonably straightforward.
nickrolfe
left a comment
There was a problem hiding this comment.
Looks good to me, other than addressing my comment about future-proofing for tracking regexp values separately from strings.
|
Performance numbers:
|
|
The failing Code Scanning check is spurious, and is being investigated. So we should be good to merge. |
This PR rewrites the
getConstantValueimplementation, to avoid bad performance arising from unnecessary recursion throughnewtypeconstructors. Previously, we had predicatesdefined (mutually) recursively by virtual dispatch. However, this meant we had to populate the
newtypeunderlyingConstantValueas part of the recursion, since QL does not permit unboundnewtypes, which in turn adds artificial (non-linear) recursion through the constructors in each predicate.In the new implementation, we instead define the following three predicates, for each type
TofConstantValue:and then the result of those is used to define the
newtype, and the user-facinggetConstantValuepredicates, but only after the recursive constant propagation computation. (TheisTExprNoCfgis not related to the performance optimization.)One down-side of this approach is that we have to replicate logic for each type
T, for example,isIntandisStringboth have to replicate how constants propagate through assignments. We may be able to get rid of some of this duplication using parameterized modules, though.