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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel accepted this revision.
ymandel added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:41
+  ///
+  ///  `Slvr` must not be null.
+  DataflowAnalysisContext(std::unique_ptr<Solver> Slvr)
----------------
I think `S` would be easier to read. 


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:244-245
+  /// order, will return the same result. If the given boolean values represent
+  /// the
+  // same value, the result will be the value itself.
+  BoolValue &makeAnd(BoolValue &LHS, BoolValue &RHS) {
----------------



================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:253-254
+  /// order, will return the same result. If the given boolean values represent
+  /// the
+  // same value, the result will be the value itself.
+  BoolValue &makeOr(BoolValue &LHS, BoolValue &RHS) {
----------------



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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120711