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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
