[PATCH] D128363: [clang][dataflow] Implement functionality for flow condition variable substitution.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added inline comments.


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp:163
+    BoolValue &Val,
+    llvm::DenseMap<BoolValue *, BoolValue *> &SubstitutionsCache) {
+  auto It = SubstitutionsCache.find(&Val);
----------------
Could you refactor it into a free function? It does not use 'this', seems like.


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp:225-229
+  if (ConstraintsIt != FlowConditionConstraints.end()) {
+    auto &Constraints = *ConstraintsIt->second;
+    return substituteBoolVal(Constraints, SubstitutionsCache);
+  }
+  return getBoolLiteralValue(true);
----------------
It is better to put special cases (early returns) first.


================
Comment at: clang/unittests/Analysis/FlowSensitive/DataflowAnalysisContextTest.cpp:191
+  // Empty flow condition holds regardless of value of boolean X
+  EXPECT_TRUE(Context.flowConditionImplies(FC, FCIndependentOfX));
+}
----------------
Could you do the following: build explicitly a BoolValue that represents the expected result formula, and then ask the SAT solver to prove equivalence between the two? That would be a stronger test than merely checking one implication.

Same in the test below.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128363/new/

https://reviews.llvm.org/D128363