From: Eli Friedman <eli.friedman@gmail.com>
Date: Fri, 6 Sep 2013 03:13:09 +0000 (+0000)
Subject: Add self-comparison warnings for fields.
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=9c5716c30d91212dcdea5e5c9b9ded9ed2439a09;p=clang

Add self-comparison warnings for fields.

This expands very slightly what -Wtautological-compare considers to be
tautological to include implicit accesses to C++ fields and ObjC ivars.
I don't want to turn this into a full expression-identity check, but
these additions seem pretty well-contained, and maintain the theme
of checking for "x == x".

<rdar://problem/14431127>

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@190118 91177308-0d34-0410-b5e6-96231b3b80d8
---

diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp
index 464d5877e8..5339c23c03 100644
--- a/lib/Sema/SemaExpr.cpp
+++ b/lib/Sema/SemaExpr.cpp
@@ -7472,6 +7472,22 @@ static void diagnoseLogicalNotOnLHSofComparison(Sema &S, ExprResult &LHS,
       << FixItHint::CreateInsertion(SecondClose, ")");
 }
 
+// Get the decl for a simple expression: a reference to a variable,
+// an implicit C++ field reference, or an implicit ObjC ivar reference.
+static ValueDecl *getCompareDecl(Expr *E) {
+  if (DeclRefExpr* DR = dyn_cast<DeclRefExpr>(E))
+    return DR->getDecl();
+  if (ObjCIvarRefExpr* Ivar = dyn_cast<ObjCIvarRefExpr>(E)) {
+    if (Ivar->isFreeIvar())
+      return Ivar->getDecl();
+  }
+  if (MemberExpr* Mem = dyn_cast<MemberExpr>(E)) {
+    if (Mem->isImplicitAccess())
+      return Mem->getMemberDecl();
+  }
+  return 0;
+}
+
 // C99 6.5.8, C++ [expr.rel]
 QualType Sema::CheckCompareOperands(ExprResult &LHS, ExprResult &RHS,
                                     SourceLocation Loc, unsigned OpaqueOpc,
@@ -7508,37 +7524,34 @@ QualType Sema::CheckCompareOperands(ExprResult &LHS, ExprResult &RHS,
     // obvious cases in the definition of the template anyways. The idea is to
     // warn when the typed comparison operator will always evaluate to the same
     // result.
-    if (DeclRefExpr* DRL = dyn_cast<DeclRefExpr>(LHSStripped)) {
-      if (DeclRefExpr* DRR = dyn_cast<DeclRefExpr>(RHSStripped)) {
-        if (DRL->getDecl() == DRR->getDecl() &&
-            !IsWithinTemplateSpecialization(DRL->getDecl())) {
-          DiagRuntimeBehavior(Loc, 0, PDiag(diag::warn_comparison_always)
-                              << 0 // self-
-                              << (Opc == BO_EQ
-                                  || Opc == BO_LE
-                                  || Opc == BO_GE));
-        } else if (LHSType->isArrayType() && RHSType->isArrayType() &&
-                   !DRL->getDecl()->getType()->isReferenceType() &&
-                   !DRR->getDecl()->getType()->isReferenceType()) {
-            // what is it always going to eval to?
-            char always_evals_to;
-            switch(Opc) {
-            case BO_EQ: // e.g. array1 == array2
-              always_evals_to = 0; // false
-              break;
-            case BO_NE: // e.g. array1 != array2
-              always_evals_to = 1; // true
-              break;
-            default:
-              // best we can say is 'a constant'
-              always_evals_to = 2; // e.g. array1 <= array2
-              break;
-            }
-            DiagRuntimeBehavior(Loc, 0, PDiag(diag::warn_comparison_always)
-                                << 1 // array
-                                << always_evals_to);
+    ValueDecl *DL = getCompareDecl(LHSStripped);
+    ValueDecl *DR = getCompareDecl(RHSStripped);
+    if (DL && DR && DL == DR && !IsWithinTemplateSpecialization(DL)) {
+      DiagRuntimeBehavior(Loc, 0, PDiag(diag::warn_comparison_always)
+                          << 0 // self-
+                          << (Opc == BO_EQ
+                              || Opc == BO_LE
+                              || Opc == BO_GE));
+    } else if (DL && DR && LHSType->isArrayType() && RHSType->isArrayType() &&
+               !DL->getType()->isReferenceType() &&
+               !DR->getType()->isReferenceType()) {
+        // what is it always going to eval to?
+        char always_evals_to;
+        switch(Opc) {
+        case BO_EQ: // e.g. array1 == array2
+          always_evals_to = 0; // false
+          break;
+        case BO_NE: // e.g. array1 != array2
+          always_evals_to = 1; // true
+          break;
+        default:
+          // best we can say is 'a constant'
+          always_evals_to = 2; // e.g. array1 <= array2
+          break;
         }
-      }
+        DiagRuntimeBehavior(Loc, 0, PDiag(diag::warn_comparison_always)
+                            << 1 // array
+                            << always_evals_to);
     }
 
     if (isa<CastExpr>(LHSStripped))
diff --git a/test/SemaCXX/self-comparison.cpp b/test/SemaCXX/self-comparison.cpp
new file mode 100644
index 0000000000..fb15ec8430
--- /dev/null
+++ b/test/SemaCXX/self-comparison.cpp
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+int foo(int x) {
+  return x == x; // expected-warning {{self-comparison always evaluates to true}}
+}
+
+struct X {
+  bool operator==(const X &x);
+};
+
+struct A {
+  int x;
+  X x2;
+  int a[3];
+  int b[3];
+  bool f() { return x == x; } // expected-warning {{self-comparison always evaluates to true}}
+  bool g() { return x2 == x2; } // no-warning
+  bool h() { return a == b; } // expected-warning {{array comparison always evaluates to false}}
+  bool i() {
+    int c[3];
+    return a == c; // expected-warning {{array comparison always evaluates to false}}
+  }
+};
diff --git a/test/SemaObjC/self-comparison.m b/test/SemaObjC/self-comparison.m
new file mode 100644
index 0000000000..00137ee0b5
--- /dev/null
+++ b/test/SemaObjC/self-comparison.m
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-objc-root-class %s
+
+@interface A {
+  id xxx;
+}
+-(int)bar;
+@end
+@implementation A
+-(int)bar {
+  return xxx == xxx; // expected-warning {{self-comparison always evaluates to true}}
+}
+@end