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
diff --git a/include/clang/Basic/DiagnosticGroups.td b/include/clang/Basic/DiagnosticGroups.td
index e3f8dbe..e7b0616 100644
--- a/include/clang/Basic/DiagnosticGroups.td
+++ b/include/clang/Basic/DiagnosticGroups.td
@@ -146,7 +146,6 @@
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 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 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 : 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 aa87808..13f3dbc 100644
--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3627,15 +3627,6 @@
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_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 79e317b..7219718 100644
--- a/lib/Sema/SemaStmt.cpp
+++ b/lib/Sema/SemaStmt.cpp
@@ -92,66 +92,17 @@
}
}
-/// \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 @@
} 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 @@
S.Diag(Loc, diag::note_equality_comparison_to_assign)
<< FixItHint::CreateReplacement(Loc, "=");
}
+
+ return true;
}
void Sema::DiagnoseUnusedExprResult(const Stmt *S) {
@@ -217,6 +164,9 @@
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 @@
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 9949887..9761116 100644
--- a/test/Sema/unused-expr.c
+++ b/test/Sema/unused-expr.c
@@ -7,11 +7,11 @@
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 @@
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 dad21ae..0000000
--- 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 0000000..79a644c
--- /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 ef0a763..0c4656a 100644
--- a/test/SemaTemplate/resolve-single-template-id.cpp
+++ b/test/SemaTemplate/resolve-single-template-id.cpp
@@ -44,9 +44,10 @@
!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 @@
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}}
}
