[cfe-commits] r137819 - in /cfe/trunk: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaStmt.cpp test/SemaCXX/warn-top-level-comparison.cpp
Chandler Carruth
chandlerc at gmail.com
Wed Aug 17 01:38:04 PDT 2011
Author: chandlerc
Date: Wed Aug 17 03:38:04 2011
New Revision: 137819
URL: http://llvm.org/viewvc/llvm-project?rev=137819&view=rev
Log:
Introduce a new warning, -Wtop-level-comparison. This warning is
a complement to the warnings we provide in condition expressions. Much
like we warn on conditions such as:
int x, y;
...
if (x = y) ... // Almost always a typo of '=='
This warning applies the complementary logic to "top-level" statements,
or statements whose value is not consumed or used in some way:
int x, y;
...
x == y; // Almost always a type for '='
We also mirror the '!=' vs. '|=' logic.
The warning is designed to fire even for overloaded operators for two reasons:
1) Especially in the presence of widespread templates that assume
operator== and operator!= perform the expected comparison operations,
it seems unreasonable to suppress warnings on the offchance that
a user has written a class that abuses these operators, embedding
side-effects or other magic within them.
2) There is a trivial source modification to silence the warning for
truly exceptional cases:
(void)(x == y); // No warning
A (greatly reduced) form of this warning has already caught a number of
bugs in our codebase, so there is precedent for it actually firing. That
said, its currently off by default, but enabled under -Wall.
There are several fixmes left here that I'm working on in follow-up
patches, including de-duplicating warnings from -Wunused, sharing code
with -Wunused's implementation (and creating a nice place to hook
diagnostics on "top-level" statements), and handling cases where a proxy
object with a bool conversion is returned, hiding the operation in the
cleanup AST nodes.
Suggestions for any of this code more than welcome. Also, I'd really
love suggestions for better naming than "top-level".
Added:
cfe/trunk/test/SemaCXX/warn-top-level-comparison.cpp
Modified:
cfe/trunk/include/clang/Basic/DiagnosticGroups.td
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/lib/Sema/SemaStmt.cpp
Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=137819&r1=137818&r2=137819&view=diff
==============================================================================
--- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Wed Aug 17 03:38:04 2011
@@ -146,6 +146,7 @@
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">;
@@ -278,8 +279,8 @@
// Thread Safety warnings
def : DiagGroup<"thread-safety">;
-// -Wall is -Wmost -Wparentheses
-def : DiagGroup<"all", [Most, Parentheses]>;
+// -Wall is -Wmost -Wparentheses -Wtop-level-comparison
+def : DiagGroup<"all", [Most, Parentheses, TopLevelComparison]>;
// Aliases.
def : DiagGroup<"", [Extra]>; // -W = -Wextra
Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=137819&r1=137818&r2=137819&view=diff
==============================================================================
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Wed Aug 17 03:38:04 2011
@@ -3627,6 +3627,15 @@
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;
Modified: cfe/trunk/lib/Sema/SemaStmt.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaStmt.cpp?rev=137819&r1=137818&r2=137819&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaStmt.cpp (original)
+++ cfe/trunk/lib/Sema/SemaStmt.cpp Wed Aug 17 03:38:04 2011
@@ -92,6 +92,103 @@
}
}
+/// \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.
+///
+/// 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;
+ }
+ }
+
+ SourceLocation Loc;
+ bool IsNotEqual = false;
+
+ if (const BinaryOperator *Op = dyn_cast<BinaryOperator>(E)) {
+ if (Op->getOpcode() != BO_EQ && Op->getOpcode() != BO_NE)
+ return;
+
+ IsNotEqual = Op->getOpcode() == BO_NE;
+ Loc = Op->getOperatorLoc();
+ } else if (const CXXOperatorCallExpr *Op = dyn_cast<CXXOperatorCallExpr>(E)) {
+ if (Op->getOperator() != OO_EqualEqual &&
+ Op->getOperator() != OO_ExclaimEqual)
+ return;
+
+ IsNotEqual = Op->getOperator() == OO_ExclaimEqual;
+ Loc = Op->getOperatorLoc();
+ } else {
+ // Not a typo-prone comparison.
+ return;
+ }
+
+ // Suppress warnings when the operator, suspicious as it may be, comes from
+ // a macro expansion.
+ if (Loc.isMacroID())
+ return;
+
+ S.Diag(Loc, diag::warn_comparison_top_level_stmt)
+ << (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 (IsNotEqual)
+ S.Diag(Loc, diag::note_inequality_comparison_to_or_assign)
+ << FixItHint::CreateReplacement(Loc, "|=");
+ else
+ S.Diag(Loc, diag::note_equality_comparison_to_assign)
+ << FixItHint::CreateReplacement(Loc, "=");
+}
+
void Sema::DiagnoseUnusedExprResult(const Stmt *S) {
if (const LabelStmt *Label = dyn_cast_or_null<LabelStmt>(S))
return DiagnoseUnusedExprResult(Label->getSubStmt());
@@ -200,6 +297,10 @@
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]);
}
Added: cfe/trunk/test/SemaCXX/warn-top-level-comparison.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-top-level-comparison.cpp?rev=137819&view=auto
==============================================================================
--- cfe/trunk/test/SemaCXX/warn-top-level-comparison.cpp (added)
+++ cfe/trunk/test/SemaCXX/warn-top-level-comparison.cpp Wed Aug 17 03:38:04 2011
@@ -0,0 +1,88 @@
+// 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}}
+ 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}}
+ 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
+}
More information about the cfe-commits
mailing list