ymandel 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(); ---------------- 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 ================ Comment at: clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp:1202 + + EXPECT_FALSE(Env.flowConditionImplies(FooVal)); + }); ---------------- 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. 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
