[PATCH] D130270: [clang][dataflow] Use a dedicated bool to encode which branch was taken

[Sam Estep via Phabricator via cfe-commits]

samestep added inline comments.


================
Comment at: clang/unittests/Analysis/FlowSensitive/DataflowAnalysisContextTest.cpp:439
   Context.addFlowConditionConstraint(FC2, Y);
-  // JoinedFC = (FC1 || FC2) && Z = (X || Y) && Z
-  auto &JoinedFC = Context.joinFlowConditions(FC1, FC2);
+  // JoinedFC = ((!B && FC1) || (B && FC2)) && Z = ((!B && X) || (B && Y)) && Z
+  auto &B = Context.createAtomicBoolValue();
----------------
ymandel wrote:
> I'm a bit concerned by the need to update this (essentially) unrelated test. It implies the test is depending on implementation details that it shouldn't be concerned with.
> 
> Not for this patch, but if you have thoughts on how to simplify this test to do what it needs w/o being tied to the details of how joins work, that would be appreciated. At least, it seems worth a FIXME.
> 
> thanks
Agreed, this test seems a bit awkward; in this patch I was just focusing on updating it instead of rethinking it.


================
Comment at: clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp:1202
+
+        EXPECT_FALSE(Env.flowConditionImplies(FooVal));
+      });
----------------
ymandel wrote:
> should we also test the inverse?
> ```
> EXPECT_FALSE(Env.flowConditionImplies(Env.makeNot(FooVal)));
> ```
> That is, I would think we want to show that neither is provable.
Sure, that makes sense; if I remember correctly, the inverse of `FooVal` is indeed not provable already. You're right that it would be good to test both here, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130270