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

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:41
+  ///
+  ///  `Slvr` must not be null.
+  DataflowAnalysisContext(std::unique_ptr<Solver> Slvr)
----------------
xazax.hun wrote:
> 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.
Agreed.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:247
+  BoolValue &makeAnd(BoolValue &LHS, BoolValue &RHS) {
+    return &LHS == &RHS ? LHS : DACtx->getOrCreateConjunctionValue(LHS, RHS);
+  }
----------------
xazax.hun wrote:
> 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`? 
Done, but kept the implication and bi-conditional helpers in `Environment` as I'm not sure if we'll need them in `DataflowAnalysisContext` at this point.


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp:30
+
+  It = ConjunctionVals.find({&RHS, &LHS});
+  if (It != ConjunctionVals.end())
----------------
xazax.hun wrote:
> 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).
Agreed. I updated the implementation.


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp:34
+
+  auto Res = ConjunctionVals.try_emplace(
+      {&LHS, &RHS},
----------------
ymandel wrote:
> Why not use `try_emplace` earlier, to save of one of the lookups?
Makes sense. I updated the implementation.


================
Comment at: clang/unittests/Analysis/FlowSensitive/DataflowAnalysisContextTest.cpp:28-31
+TEST_F(DataflowAnalysisContextTest,
+       GetOrCreateConjunctionValueReturnsSameExprOnSubsequentCalls) {
+  auto &X = Context.createAtomicBoolValue();
+  auto &Y = Context.createAtomicBoolValue();
----------------
ymandel wrote:
> maybe add another one which simply tests that two calls to `createAtomicBoolValue` return distinct values (or even just distinct pointers?)
Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120711