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.

llvm-svn: 137822
This commit is contained in:
Chandler Carruth 2011-08-17 09:34:37 +00:00
parent 29027b9352
commit e2669397f1
7 changed files with 108 additions and 184 deletions

View File

@ -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

View File

@ -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">;

View File

@ -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]);
}

View File

@ -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

View File

@ -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
}

View File

@ -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
}

View File

@ -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}}
}