Treating the unused equality comparisons as something other than part of

-Wunused was a mistake. It resulted in duplicate warnings and lots of
other hacks. Instead, this should be a special sub-category to
-Wunused-value, much like -Wunused-result is.

Moved to -Wunused-comparison, moved the implementation to piggy back on
the -Wunused-value implementation instead of rolling its own, different
mechanism for catching all of the "interesting" statements.

I like the unused-value mechanism for this better, but its currently
missing several top-level statements. For now, I've FIXME-ed out those
test cases. I'll enhance the generic infrastructure to catch these
statements in a subsequent patch.

This patch also removes the cast-to-void fixit hint. This hint isn't
available on any of the other -Wunused-value diagnostics, and if we want
it to be, we should add it generically rather than in one specific case.

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

7 files changed, 108 insertions, 184 deletions

diff --git a/include/clang/Basic/DiagnosticGroups.td b/include/clang/Basic/DiagnosticGroups.td
index e3f8dbe197..e7b061647c 100644
--- a/include/clang/Basic/DiagnosticGroups.td
+++ b/include/clang/Basic/DiagnosticGroups.td
@@ -146,7 +146,6 @@ def : DiagGroup<"strict-prototypes">;
 def StrictSelector : DiagGroup<"strict-selector-match">;
 def SwitchEnum     : DiagGroup<"switch-enum">;
 def Switch         : DiagGroup<"switch", [SwitchEnum]>;
-def TopLevelComparison : DiagGroup<"top-level-comparison">;
 def Trigraphs      : DiagGroup<"trigraphs">;
 
 def : DiagGroup<"type-limits">;
@@ -156,6 +155,7 @@ def UnknownPragmas : DiagGroup<"unknown-pragmas">;
 def UnknownAttributes : DiagGroup<"attributes">;
 def UnnamedTypeTemplateArgs : DiagGroup<"unnamed-type-template-args">;
 def UnusedArgument : DiagGroup<"unused-argument">;
+def UnusedComparison : DiagGroup<"unused-comparison">;
 def UnusedExceptionParameter : DiagGroup<"unused-exception-parameter">;
 def UnneededInternalDecl : DiagGroup<"unneeded-internal-declaration">;
 def UnneededMemberFunction : DiagGroup<"unneeded-member-function">;
@@ -165,7 +165,7 @@ def UnusedMemberFunction : DiagGroup<"unused-member-function",
 def UnusedLabel : DiagGroup<"unused-label">;
 def UnusedParameter : DiagGroup<"unused-parameter">;
 def UnusedResult : DiagGroup<"unused-result">;
-def UnusedValue    : DiagGroup<"unused-value", [UnusedResult]>;
+def UnusedValue : DiagGroup<"unused-value", [UnusedComparison, UnusedResult]>;
 def UnusedVariable : DiagGroup<"unused-variable">;
 def UsedButMarkedUnused : DiagGroup<"used-but-marked-unused">;
 def ReadOnlySetterAttrs : DiagGroup<"readonly-setter-attrs">;
@@ -280,7 +280,7 @@ def Most : DiagGroup<"most", [
 def : DiagGroup<"thread-safety">;
 
 // -Wall is -Wmost -Wparentheses -Wtop-level-comparison
-def : DiagGroup<"all", [Most, Parentheses, TopLevelComparison]>;
+def : DiagGroup<"all", [Most, Parentheses]>;
 
 // Aliases.
 def : DiagGroup<"", [Extra]>;                   // -W = -Wextra
diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
index aa8780821b..13f3dbc1d4 100644
--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3627,15 +3627,6 @@ def note_equality_comparison_to_assign : Note<
 def note_equality_comparison_silence : Note<
   "remove extraneous parentheses around the comparison to silence this warning">;
 
-def warn_comparison_top_level_stmt : Warning<
-  "%select{equality|inequality}0 comparison as an unused top-level statement">,
-  InGroup<TopLevelComparison>, DefaultIgnore;
-def note_inequality_comparison_to_or_assign : Note<
-  "use '|=' to turn this inequality comparison into an or-assignment">;
-def note_top_level_comparison_void_cast_silence : Note<
-  "cast this comparison to void to silence this warning">;
-
-
 def warn_synthesized_ivar_access : Warning<
   "direct access of synthesized ivar by using property access %0">,
   InGroup<NonfragileAbi2>, DefaultIgnore;
@@ -3884,6 +3875,11 @@ def warn_unused_call : Warning<
 def warn_unused_result : Warning<
   "ignoring return value of function declared with warn_unused_result "
   "attribute">, InGroup<DiagGroup<"unused-result">>;
+def warn_unused_comparison : Warning<
+  "%select{equality|inequality}0 comparison result unused">,
+  InGroup<UnusedComparison>;
+def note_inequality_comparison_to_or_assign : Note<
+  "use '|=' to turn this inequality comparison into an or-assignment">;
 
 def err_incomplete_type_used_in_type_trait_expr : Error<
   "incomplete type %0 used in type trait expression">;
diff --git a/lib/Sema/SemaStmt.cpp b/lib/Sema/SemaStmt.cpp
index 79e317bb8c..7219718ba4 100644
--- a/lib/Sema/SemaStmt.cpp
+++ b/lib/Sema/SemaStmt.cpp
@@ -92,66 +92,17 @@ void Sema::ActOnForEachDeclStmt(DeclGroupPtrTy dg) {
   }
 }
 
-/// \brief Diagnose '==' and '!=' in top-level statements as likely
-/// typos for '=' or '|=' (resp.).
-///
-/// This function looks through common stand-alone statements to dig out the
-/// substatement of interest. It should be viable to call on any direct member
-/// of a CompoundStmt.
+/// \brief Diagnose unused '==' and '!=' as likely typos for '=' or '|='.
 ///
 /// Adding a cast to void (or other expression wrappers) will prevent the
 /// warning from firing.
-static void DiagnoseTopLevelComparison(Sema &S, const Stmt *Statement) {
-  if (!Statement) return;
-  const Expr *E = dyn_cast<Expr>(Statement);
-  if (!E) {
-    // Descend to sub-statements where they remain "top-level" in that they're
-    // unused.
-    switch (Statement->getStmtClass()) {
-    default:
-      // Skip statements which don't have a direct substatement of interest.
-      // Compound statements are handled by the caller.
-      return;
-
-    case Stmt::CaseStmtClass:
-    case Stmt::DefaultStmtClass:
-      DiagnoseTopLevelComparison(S, cast<SwitchCase>(Statement)->getSubStmt());
-      return;
-    case Stmt::LabelStmtClass:
-      DiagnoseTopLevelComparison(S, cast<LabelStmt>(Statement)->getSubStmt());
-      return;
-
-    case Stmt::DoStmtClass:
-      DiagnoseTopLevelComparison(S, cast<DoStmt>(Statement)->getBody());
-      return;
-    case Stmt::IfStmtClass: {
-      const IfStmt *If = cast<IfStmt>(Statement);
-      DiagnoseTopLevelComparison(S, If->getThen());
-      DiagnoseTopLevelComparison(S, If->getElse());
-      return;
-    }
-    case Stmt::ForStmtClass: {
-      const ForStmt *ForLoop = cast<ForStmt>(Statement);
-      DiagnoseTopLevelComparison(S, ForLoop->getInit());
-      DiagnoseTopLevelComparison(S, ForLoop->getInc());
-      DiagnoseTopLevelComparison(S, ForLoop->getBody());
-      return;
-    }
-    case Stmt::SwitchStmtClass:
-      DiagnoseTopLevelComparison(S, cast<SwitchStmt>(Statement)->getBody());
-      return;
-    case Stmt::WhileStmtClass:
-      DiagnoseTopLevelComparison(S, cast<WhileStmt>(Statement)->getBody());
-      return;
-    }
-  }
-
+static bool DiagnoseUnusedComparison(Sema &S, const Expr *E) {
   SourceLocation Loc;
   bool IsNotEqual, CanAssign;
 
   if (const BinaryOperator *Op = dyn_cast<BinaryOperator>(E)) {
     if (Op->getOpcode() != BO_EQ && Op->getOpcode() != BO_NE)
-      return;
+      return false;
 
     Loc = Op->getOperatorLoc();
     IsNotEqual = Op->getOpcode() == BO_NE;
@@ -159,30 +110,24 @@ static void DiagnoseTopLevelComparison(Sema &S, const Stmt *Statement) {
   } else if (const CXXOperatorCallExpr *Op = dyn_cast<CXXOperatorCallExpr>(E)) {
     if (Op->getOperator() != OO_EqualEqual &&
         Op->getOperator() != OO_ExclaimEqual)
-      return;
+      return false;
 
     Loc = Op->getOperatorLoc();
     IsNotEqual = Op->getOperator() == OO_ExclaimEqual;
     CanAssign = Op->getArg(0)->IgnoreParenImpCasts()->isLValue();
   } else {
     // Not a typo-prone comparison.
-    return;
+    return false;
   }
 
   // Suppress warnings when the operator, suspicious as it may be, comes from
   // a macro expansion.
   if (Loc.isMacroID())
-    return;
+    return false;
 
-  S.Diag(Loc, diag::warn_comparison_top_level_stmt)
+  S.Diag(Loc, diag::warn_unused_comparison)
     << (unsigned)IsNotEqual << E->getSourceRange();
 
-  SourceLocation Open = E->getSourceRange().getBegin();
-  SourceLocation Close = S.PP.getLocForEndOfToken(E->getSourceRange().getEnd());
-  S.Diag(Loc, diag::note_top_level_comparison_void_cast_silence)
-        << FixItHint::CreateInsertion(Open, "(void)(")
-        << FixItHint::CreateInsertion(Close, ")");
-
   // If the LHS is a plausible entity to assign to, provide a fixit hint to
   // correct common typos.
   if (CanAssign) {
@@ -193,6 +138,8 @@ static void DiagnoseTopLevelComparison(Sema &S, const Stmt *Statement) {
       S.Diag(Loc, diag::note_equality_comparison_to_assign)
         << FixItHint::CreateReplacement(Loc, "=");
   }
+
+  return true;
 }
 
 void Sema::DiagnoseUnusedExprResult(const Stmt *S) {
@@ -217,6 +164,9 @@ void Sema::DiagnoseUnusedExprResult(const Stmt *S) {
   if (const CXXBindTemporaryExpr *TempExpr = dyn_cast<CXXBindTemporaryExpr>(E))
     E = TempExpr->getSubExpr();
 
+  if (DiagnoseUnusedComparison(*this, E))
+    return;
+
   E = E->IgnoreParenImpCasts();
   if (const CallExpr *CE = dyn_cast<CallExpr>(E)) {
     if (E->getType()->isVoidType())
@@ -303,10 +253,6 @@ Sema::ActOnCompoundStmt(SourceLocation L, SourceLocation R,
     if (isStmtExpr && i == NumElts - 1)
       continue;
 
-    // FIXME: It'd be nice to not show both of these. The first is a more
-    // precise (and more likely to be enabled) warning. We should suppress the
-    // second when the first fires.
-    DiagnoseTopLevelComparison(*this, Elts[i]);
     DiagnoseUnusedExprResult(Elts[i]);
   }
 
diff --git a/test/Sema/unused-expr.c b/test/Sema/unused-expr.c
index 9949887b23..97611168f1 100644
--- a/test/Sema/unused-expr.c
+++ b/test/Sema/unused-expr.c
@@ -7,11 +7,11 @@ double sqrt(double X);  // implicitly const because of no -fmath-errno!
 void bar(volatile int *VP, int *P, int A,
          _Complex double C, volatile _Complex double VC) {
   
-  VP == P;             // expected-warning {{expression result unused}}
+  VP < P;              // expected-warning {{expression result unused}}
   (void)A;
   (void)foo(1,2);      // no warning.
   
-  A == foo(1, 2);      // expected-warning {{expression result unused}}
+  A < foo(1, 2);       // expected-warning {{expression result unused}}
 
   foo(1,2)+foo(4,3);   // expected-warning {{expression result unused}}
 
@@ -54,23 +54,23 @@ void t4(int a) {
   int b = 0;
 
   if (a)
-    b == 1; // expected-warning{{expression result unused}}
+    b < 1; // expected-warning{{expression result unused}}
   else
-    b == 2; // expected-warning{{expression result unused}}
+    b < 2; // expected-warning{{expression result unused}}
     
   while (1)
-    b == 3; // expected-warning{{expression result unused}}
+    b < 3; // expected-warning{{expression result unused}}
 
   do
-    b == 4; // expected-warning{{expression result unused}}
+    b < 4; // expected-warning{{expression result unused}}
   while (1);
   
   for (;;)
-    b == 5; // expected-warning{{expression result unused}}
+    b < 5; // expected-warning{{expression result unused}}
     
-  for (b == 1;;) {} // expected-warning{{expression result unused}}
-  for (;b == 1;) {}
-  for (;;b == 1) {} // expected-warning{{expression result unused}}
+  for (b < 1;;) {} // expected-warning{{expression result unused}}
+  for (;b < 1;) {}
+  for (;;b < 1) {} // expected-warning{{expression result unused}}
 }
 
 // rdar://7186119
diff --git a/test/SemaCXX/warn-top-level-comparison.cpp b/test/SemaCXX/warn-top-level-comparison.cpp
deleted file mode 100644
index dad21aeedc..0000000000
--- a/test/SemaCXX/warn-top-level-comparison.cpp
+++ /dev/null
@@ -1,92 +0,0 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -Wtop-level-comparison -Wno-unused %s
-
-struct A {
-  bool operator==(const A&);
-  bool operator!=(const A&);
-  A operator|=(const A&);
-  operator bool();
-};
-
-void test() {
-  int x, *p;
-  A a, b;
-
-  x == 7; // expected-warning {{equality comparison as an unused top-level statement}} \
-          // expected-note {{cast this comparison to void to silence this warning}} \
-          // expected-note {{use '=' to turn this equality comparison into an assignment}}
-  x != 7; // expected-warning {{inequality comparison as an unused top-level statement}} \
-          // expected-note {{cast this comparison to void to silence this warning}} \
-          // expected-note {{use '|=' to turn this inequality comparison into an or-assignment}}
-  7 == x; // expected-warning {{equality comparison as an unused top-level statement}} \
-          // expected-note {{cast this comparison to void to silence this warning}}
-  p == p; // expected-warning {{equality comparison as an unused top-level statement}} \
-          // expected-note {{cast this comparison to void to silence this warning}} \
-          // expected-note {{use '=' to turn this equality comparison into an assignment}} \
-          // expected-warning {{self-comparison always evaluates to true}}
-  a == a; // expected-warning {{equality comparison as an unused top-level statement}} \
-          // expected-note {{cast this comparison to void to silence this warning}} \
-          // expected-note {{use '=' to turn this equality comparison into an assignment}}
-  a == b; // expected-warning {{equality comparison as an unused top-level statement}} \
-          // expected-note {{cast this comparison to void to silence this warning}} \
-          // expected-note {{use '=' to turn this equality comparison into an assignment}}
-  a != b; // expected-warning {{inequality comparison as an unused top-level statement}} \
-          // expected-note {{cast this comparison to void to silence this warning}} \
-          // expected-note {{use '|=' to turn this inequality comparison into an or-assignment}}
-  A() == b; // expected-warning {{equality comparison as an unused top-level statement}} \
-            // expected-note {{cast this comparison to void to silence this warning}}
-  if (42) x == 7; // expected-warning {{equality comparison as an unused top-level statement}} \
-                  // expected-note {{cast this comparison to void to silence this warning}} \
-                  // expected-note {{use '=' to turn this equality comparison into an assignment}}
-  else if (42) x == 7; // expected-warning {{equality comparison as an unused top-level statement}} \
-                       // expected-note {{cast this comparison to void to silence this warning}} \
-                       // expected-note {{use '=' to turn this equality comparison into an assignment}}
-  else x == 7; // expected-warning {{equality comparison as an unused top-level statement}} \
-               // expected-note {{cast this comparison to void to silence this warning}} \
-               // expected-note {{use '=' to turn this equality comparison into an assignment}}
-  do x == 7; // expected-warning {{equality comparison as an unused top-level statement}} \
-             // expected-note {{cast this comparison to void to silence this warning}} \
-             // expected-note {{use '=' to turn this equality comparison into an assignment}}
-  while (false);
-  while (false) x == 7; // expected-warning {{equality comparison as an unused top-level statement}} \
-                        // expected-note {{cast this comparison to void to silence this warning}} \
-                        // expected-note {{use '=' to turn this equality comparison into an assignment}}
-  for (x == 7; // expected-warning {{equality comparison as an unused top-level statement}} \
-               // expected-note {{cast this comparison to void to silence this warning}} \
-               // expected-note {{use '=' to turn this equality comparison into an assignment}}
-       x == 7; // No warning -- result is used
-       x == 7) // expected-warning {{equality comparison as an unused top-level statement}} \
-               // expected-note {{cast this comparison to void to silence this warning}} \
-               // expected-note {{use '=' to turn this equality comparison into an assignment}}
-    x == 7; // expected-warning {{equality comparison as an unused top-level statement}} \
-            // expected-note {{cast this comparison to void to silence this warning}} \
-            // expected-note {{use '=' to turn this equality comparison into an assignment}}
-  switch (42) default: x == 7; // expected-warning {{equality comparison as an unused top-level statement}} \
-                               // expected-note {{cast this comparison to void to silence this warning}} \
-                               // expected-note {{use '=' to turn this equality comparison into an assignment}}
-  switch (42) case 42: x == 7; // expected-warning {{equality comparison as an unused top-level statement}} \
-                               // expected-note {{cast this comparison to void to silence this warning}} \
-                               // expected-note {{use '=' to turn this equality comparison into an assignment}}
-  switch (42) {
-    case 1:
-    case 2:
-    default:
-    case 3:
-    case 4:
-      x == 7; // expected-warning {{equality comparison as an unused top-level statement}} \
-              // expected-note {{cast this comparison to void to silence this warning}} \
-              // expected-note {{use '=' to turn this equality comparison into an assignment}}
-  }
-
-  (void)(x == 7);
-  (void)(p == p); // expected-warning {{self-comparison always evaluates to true}}
-  { bool b = x == 7; }
-
-  { bool b = ({ x == 7; // expected-warning {{equality comparison as an unused top-level statement}} \
-                        // expected-note {{cast this comparison to void to silence this warning}} \
-                        // expected-note {{use '=' to turn this equality comparison into an assignment}}
-                x == 7; }); } // no warning on the second, its result is used!
-
-#define EQ(x,y) (x) == (y)
-  EQ(x, 5);
-#undef EQ
-}
diff --git a/test/SemaCXX/warn-unused-comparison.cpp b/test/SemaCXX/warn-unused-comparison.cpp
new file mode 100644
index 0000000000..79a644cefc
--- /dev/null
+++ b/test/SemaCXX/warn-unused-comparison.cpp
@@ -0,0 +1,72 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-unused -Wunused-comparison %s
+
+struct A {
+  bool operator==(const A&);
+  bool operator!=(const A&);
+  A operator|=(const A&);
+  operator bool();
+};
+
+void test() {
+  int x, *p;
+  A a, b;
+
+  x == 7; // expected-warning {{equality comparison result unused}} \
+          // expected-note {{use '=' to turn this equality comparison into an assignment}}
+  x != 7; // expected-warning {{inequality comparison result unused}} \
+          // expected-note {{use '|=' to turn this inequality comparison into an or-assignment}}
+  7 == x; // expected-warning {{equality comparison result unused}}
+  p == p; // expected-warning {{equality comparison result unused}} \
+          // expected-note {{use '=' to turn this equality comparison into an assignment}} \
+          // expected-warning {{self-comparison always evaluates to true}}
+  a == a; // FIXME: missing-warning {{equality comparison result unused}} \
+          // FIXME: missing-note {{use '=' to turn this equality comparison into an assignment}}
+  a == b; // FIXME: missing-warning {{equality comparison result unused}} \
+          // FIXME: missing-note {{use '=' to turn this equality comparison into an assignment}}
+  a != b; // FIXME: missing-warning {{inequality comparison result unused}} \
+          // FIXME: missing-note {{use '|=' to turn this inequality comparison into an or-assignment}}
+  A() == b; // FIXME: missing-warning {{equality comparison result unused}}
+  if (42) x == 7; // expected-warning {{equality comparison result unused}} \
+                  // expected-note {{use '=' to turn this equality comparison into an assignment}}
+  else if (42) x == 7; // expected-warning {{equality comparison result unused}} \
+                       // expected-note {{use '=' to turn this equality comparison into an assignment}}
+  else x == 7; // expected-warning {{equality comparison result unused}} \
+               // expected-note {{use '=' to turn this equality comparison into an assignment}}
+  do x == 7; // expected-warning {{equality comparison result unused}} \
+             // expected-note {{use '=' to turn this equality comparison into an assignment}}
+  while (false);
+  while (false) x == 7; // expected-warning {{equality comparison result unused}} \
+                        // expected-note {{use '=' to turn this equality comparison into an assignment}}
+  for (x == 7; // expected-warning {{equality comparison result unused}} \
+               // expected-note {{use '=' to turn this equality comparison into an assignment}}
+       x == 7; // No warning -- result is used
+       x == 7) // expected-warning {{equality comparison result unused}} \
+               // expected-note {{use '=' to turn this equality comparison into an assignment}}
+    x == 7; // expected-warning {{equality comparison result unused}} \
+            // expected-note {{use '=' to turn this equality comparison into an assignment}}
+  switch (42) default: x == 7; // FIXME: missing-warning {{equality comparison result unused}} \
+                               // FIXME: missing-note {{use '=' to turn this equality comparison into an assignment}}
+  switch (42) case 42: x == 7; // FIXME: missing-warning {{equality comparison result unused}} \
+                               // FIXME: missing-note {{use '=' to turn this equality comparison into an assignment}}
+  switch (42) {
+    case 1:
+    case 2:
+    default:
+    case 3:
+    case 4:
+      x == 7; // FIXME: missing-warning {{equality comparison result unused}} \
+              // FIXME: missing-note {{use '=' to turn this equality comparison into an assignment}}
+  }
+
+  (void)(x == 7);
+  (void)(p == p); // expected-warning {{self-comparison always evaluates to true}}
+  { bool b = x == 7; }
+
+  { bool b = ({ x == 7; // expected-warning {{equality comparison result unused}} \
+                        // expected-note {{use '=' to turn this equality comparison into an assignment}}
+                x == 7; }); } // no warning on the second, its result is used!
+
+#define EQ(x,y) (x) == (y)
+  EQ(x, 5);
+#undef EQ
+}
diff --git a/test/SemaTemplate/resolve-single-template-id.cpp b/test/SemaTemplate/resolve-single-template-id.cpp
index ef0a763076..0c4656a8bc 100644
--- a/test/SemaTemplate/resolve-single-template-id.cpp
+++ b/test/SemaTemplate/resolve-single-template-id.cpp
@@ -44,9 +44,10 @@ int main()
   !oneT<int>;  // expected-warning {{expression result unused}}
   +oneT<int>;  // expected-warning {{expression result unused}}
   -oneT<int>;  //expected-error {{invalid argument type}}
-  oneT<int> == 0;   // expected-warning {{expression result unused}}
-  0 == oneT<int>;   // expected-warning {{expression result unused}}
-  0 != oneT<int>;    // expected-warning {{expression result unused}}
+  oneT<int> == 0;   // expected-warning {{equality comparison result unused}} \
+                    // expected-note {{use '=' to turn this equality comparison into an assignment}}
+  0 == oneT<int>;   // expected-warning {{equality comparison result unused}}
+  0 != oneT<int>;    // expected-warning {{inequality comparison result unused}}
   (false ? one : oneT<int>);   // expected-warning {{expression result unused}}
   void (*p1)(int); p1 = oneT<int>;
   
@@ -67,7 +68,8 @@ int main()
 
   two < two; //expected-error {{cannot resolve overloaded function 'two' from context}}
   twoT<int> < twoT<int>; //expected-error {{cannot resolve overloaded function 'twoT' from context}}
-  oneT<int> == 0;   // expected-warning {{expression result unused}}
+  oneT<int> == 0;   // expected-warning {{equality comparison result unused}} \
+                    // expected-note {{use '=' to turn this equality comparison into an assignment}}
 
 }