[PATCH] D66045: Improve detection of same value in comparisons

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: lib/AST/Expr.cpp:3931-3932
+    case DeclRefExprClass: {
+      // DeclRefExpr without an ImplicitCastExpr can happen for integral
+      // template parameters.
+      const auto *DRE1 = cast<DeclRefExpr>(E1);
----------------
Does this actually happen *after* instantiation? If not - do we even need to emit warnings on templates that aren't instantiated? I guess we still need this branch because `isSameComparisonOperand()` lives in the `Expr` class and should always work regardless, but in this case i guess this deserves a unittest(?)


================
Comment at: lib/AST/Expr.cpp:3976
+        if (const auto *D = dyn_cast<VarDecl>(ME1->getMemberDecl()))
+          if (D->isStaticDataMember())
+            return true;
----------------
Mmm, what are other cases when a `MemberExpr` doesn't have a `FieldDecl` as its `getMemberDecl()`? Can this be turned into an `assert`?


================
Comment at: test/Analysis/array-struct-region.cpp:31
+  bool check() const { return this == this + 0; }
+  bool operator !() const { return this != this + 0; }
 
----------------
rtrieu wrote:
> jfb wrote:
> > Is this the only way? Or do casts also disable the diagnostic?
> There's other ways to do this.  I picked "+ 0" here.  Explicit casts, like to void* or const S* would also disable it.  I think pragmas would also work.  I don't particularly care which way since this is an analyzer test.
I'd rather disable the newly introduced warning on this test file, because this is a path-sensitive static analysis test and this change may accidentally ruin the original test.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66045/new/

https://reviews.llvm.org/D66045