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

[Richard Trieu via Phabricator via cfe-commits]

rtrieu marked 2 inline comments as done.
rtrieu 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);
----------------
NoQ wrote:
> 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(?)
This happens before instantiation.  The test cases are in (test/SemaTemplate/self-comparison.cpp).  The basic idea is to allow integral template parameters to be treated as variables.

```
template<int A>
void foo(int x) {
  bool b1 = A == A;
  bool b2 = x == x;
}
```

If we treat template parameter A the same as variable x, we can diagnose a few more tautological types, even without instantiation.


================
Comment at: lib/AST/Expr.cpp:3976
+        if (const auto *D = dyn_cast<VarDecl>(ME1->getMemberDecl()))
+          if (D->isStaticDataMember())
+            return true;
----------------
NoQ wrote:
> Mmm, what are other cases when a `MemberExpr` doesn't have a `FieldDecl` as its `getMemberDecl()`? Can this be turned into an `assert`?
I had to look this one up too.  From the docs for this method:

```
  /// Retrieve the member declaration to which this expression refers.
  ///
  /// The returned declaration will be a FieldDecl or (in C++) a VarDecl (for
  /// static data members), a CXXMethodDecl, or an EnumConstantDecl.
  ValueDecl *getMemberDecl() const { return MemberDecl; }
```

So, FieldDecl, VarDecl, CXXMethodDecl, and EnumConstantDecl are all valid return types.


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

https://reviews.llvm.org/D66045