aaron.ballman added a comment. In D81272#2218173 <https://reviews.llvm.org/D81272#2218173>, @whisperity wrote:
> In D81272#2218050 <https://reviews.llvm.org/D81272#2218050>, @aaron.ballman > wrote: > >> Thanks to the new info, I think the check basically LGTM. Can you add some >> negative tests and documentation wording to make it clear that the check >> doesn't currently handle all logically equivalent predicates, like: >> >> if (foo) { >> } else { >> if (!foo) { >> } >> } >> >> // or >> if (foo > 5) { >> if (foo > 3) { >> } >> } >> >> // or >> if (foo > 5) { >> if (5 < foo) { >> } >> } >> >> (I'm assuming these cases aren't handled currently and that handling them >> isn't necessary to land the patch.) > > I'm afraid handling such cases will eventually invoke the same problems the > RangeConstraintSolver has, and then the discussion about whether this is good > in Tidy instead of CSA will be resurrected... 😦 > > One could come up with even more elaborate cases. Just hit something like > below a few minutes ago while working: > > const CallExpr* const C; > > if (auto* CalledFun = dyn_cast_or_null<FunctionDecl>(C->getCalleeDecl())) { > // ... > if (const auto* Fun = dyn_cast_or_null<FunctionDecl>(C->getCalleeDecl())) > { > // ... > } > // ... > } > > and the rabbit hole goes deeper. But it's pretty interesting how many cases > the current check found as-is. While I agree with your observation about data and flow sensitivity, I approved on the belief that the check as-is provides enough utility to warrant adding it as-is. If someone wants to improve the check into being a CSA check, we can always deprecate this one at that point. However, if there are strong opinions that the check should start out as a CSA check because it requires that sensitivity for your needs, now's a good time to bring up those concerns. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81272/new/ https://reviews.llvm.org/D81272 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
