[PATCH] D152093: [clang][Analysis] Handle && and || against variable and its negation as tautology

Fri Jul 14 06:20:44 PDT 2023

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