[PATCH] D120711: [clang][dataflow] Add flow condition constraints to Environment

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:41
+  ///
+  ///  `Slvr` must not be null.
+  DataflowAnalysisContext(std::unique_ptr<Solver> Slvr)
----------------
ymandel wrote:
> I think `S` would be easier to read. 
Initially, I was wondering if we want to make the solver optional to potentially speed up analyses that do not need this capability. But I realized, we could just pass in a dummy no-op solver in those cases so this should be ok.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:247
+  BoolValue &makeAnd(BoolValue &LHS, BoolValue &RHS) {
+    return &LHS == &RHS ? LHS : DACtx->getOrCreateConjunctionValue(LHS, RHS);
+  }
----------------
Should we move this check to `DataflowAnalysisContext` to protect against cases where users inadvertently try to call the its methods directly instead of using the `Environment`? 


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp:30
+
+  It = ConjunctionVals.find({&RHS, &LHS});
+  if (It != ConjunctionVals.end())
----------------
Alternatively, we could canonicalize `BoolValue`s, e.g. always having the operand with the smaller address on the left. This could help us doing one less lookup (at the cost of a pointer comparison and sometimes a swap).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120711