hazohelet added a comment. > Do you have any numbers on how often this fires in practice, and what the > true positive rate is? (Build some large-ish open source project with this, > and see what it finds.)
I built rui314/mold and saw no new warnings emitted from this change. I searched several repositories to find only one instance in CBLAS code that this change could be relevant. https://github.com/Reference-LAPACK/lapack/blob/c57e36a43bb1c25c7ec15a3c84f4800942a5ca43/CBLAS/src/cblas_xerbla.c#L69-L70 I'm not sure what exactly they are doing there, but the comment (`Force link of our F77 error handler`) suggests it is intentional, so it could be counted as false positive. But, I don't see any reason for not doing `if (0)` instead. > Did you verify that this has negligible compile time impact for building a > large-ish project? About the compile time impact, I don't expect this patch to have a noticeable change. The changes in this patch does not contain heavy computations like AST traversals. At least the build times of rui314/mold by clang ToT and patched clang did not have noticeable difference. ================ Comment at: clang/lib/Analysis/CFG.cpp:1096 + if (Negate->getOpcode() == UO_LNot && + Expr::isSameComparisonOperand(Negate->getSubExpr(), E2)) { + bool AlwaysTrue = B->getOpcode() == BO_LOr; ---------------- thakis wrote: > Do you want to IgnoreParens() on the ! subexpr too? I wanted to, but it looks like most codes containing parentheses in negation subexpression are macros, so now I don't think we need it. https://sourcegraph.com/search?q=context:global+/%21%5C%28%5Ba-zA-Z_%5D%2B%5C%29/+lang:C+lang:C%2B%2B&patternType=standard&case=yes&sm=1&groupBy=repo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152093/new/ https://reviews.llvm.org/D152093 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits