r261278 - Add -Wcomma warning to Clang.
Richard Trieu via cfe-commits
cfe-commits at lists.llvm.org
Thu Feb 18 15:58:41 PST 2016
Author: rtrieu
Date: Thu Feb 18 17:58:40 2016
New Revision: 261278
URL: http://llvm.org/viewvc/llvm-project?rev=261278&view=rev
Log:
Add -Wcomma warning to Clang.
-Wcomma will detect and warn on most uses of the builtin comma operator. It
currently whitelists the first and third statements of the for-loop. For other
cases, the warning can be silenced by casting the first operand of the comma
operator to void.
Differential Revision: http://reviews.llvm.org/D3976
Added:
cfe/trunk/test/SemaCXX/warn-comma-operator.cpp
Modified:
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/include/clang/Sema/Sema.h
cfe/trunk/lib/Sema/SemaExpr.cpp
cfe/trunk/lib/Sema/SemaStmt.cpp
Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=261278&r1=261277&r2=261278&view=diff
==============================================================================
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Feb 18 17:58:40 2016
@@ -74,6 +74,10 @@ def warn_infinite_recursive_function : W
"all paths through this function will call itself">,
InGroup<InfiniteRecursion>, DefaultIgnore;
+def warn_comma_operator : Warning<"possible misuse of comma operator here">,
+ InGroup<DiagGroup<"comma">>, DefaultIgnore;
+def note_cast_to_void : Note<"cast expression to void to silence warning">;
+
// Constant expressions
def err_expr_not_ice : Error<
"expression is not an %select{integer|integral}0 constant expression">;
Modified: cfe/trunk/include/clang/Sema/Sema.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=261278&r1=261277&r2=261278&view=diff
==============================================================================
--- cfe/trunk/include/clang/Sema/Sema.h (original)
+++ cfe/trunk/include/clang/Sema/Sema.h Thu Feb 18 17:58:40 2016
@@ -3971,6 +3971,8 @@ public:
ExprResult CreateBuiltinBinOp(SourceLocation OpLoc, BinaryOperatorKind Opc,
Expr *LHSExpr, Expr *RHSExpr);
+ void DiagnoseCommaOperator(const Expr *LHS, SourceLocation Loc);
+
/// ActOnConditionalOp - Parse a ?: operation. Note that 'LHS' may be null
/// in the case of a the GNU conditional expr extension.
ExprResult ActOnConditionalOp(SourceLocation QuestionLoc,
Modified: cfe/trunk/lib/Sema/SemaExpr.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=261278&r1=261277&r2=261278&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaExpr.cpp (original)
+++ cfe/trunk/lib/Sema/SemaExpr.cpp Thu Feb 18 17:58:40 2016
@@ -9867,6 +9867,67 @@ QualType Sema::CheckAssignmentOperands(E
? LHSType : LHSType.getUnqualifiedType());
}
+// Only ignore explicit casts to void.
+static bool IgnoreCommaOperand(const Expr *E) {
+ E = E->IgnoreParens();
+
+ if (const CastExpr *CE = dyn_cast<CastExpr>(E)) {
+ if (CE->getCastKind() == CK_ToVoid) {
+ return true;
+ }
+ }
+
+ return false;
+}
+
+// Look for instances where it is likely the comma operator is confused with
+// another operator. There is a whitelist of acceptable expressions for the
+// left hand side of the comma operator, otherwise emit a warning.
+void Sema::DiagnoseCommaOperator(const Expr *LHS, SourceLocation Loc) {
+ // No warnings in macros
+ if (Loc.isMacroID())
+ return;
+
+ // Don't warn in template instantiations.
+ if (!ActiveTemplateInstantiations.empty())
+ return;
+
+ // Scope isn't fine-grained enough to whitelist the specific cases, so
+ // instead, skip more than needed, then call back into here with the
+ // CommaVisitor in SemaStmt.cpp.
+ // The whitelisted locations are the initialization and increment portions
+ // of a for loop. The additional checks are on the condition of
+ // if statements, do/while loops, and for loops.
+ const unsigned ForIncreamentFlags =
+ Scope::ControlScope | Scope::ContinueScope | Scope::BreakScope;
+ const unsigned ForInitFlags = Scope::ControlScope | Scope::DeclScope;
+ const unsigned ScopeFlags = getCurScope()->getFlags();
+ if ((ScopeFlags & ForIncrementFlags) == ForIncrementFlags ||
+ (ScopeFlags & ForInitFlags) == ForInitFlags)
+ return;
+
+ // If there are multiple comma operators used together, get the RHS of the
+ // of the comma operator as the LHS.
+ while (const BinaryOperator *BO = dyn_cast<BinaryOperator>(LHS)) {
+ if (BO->getOpcode() != BO_Comma)
+ break;
+ LHS = BO->getRHS();
+ }
+
+ // Only allow some expressions on LHS to not warn.
+ if (IgnoreCommaOperand(LHS))
+ return;
+
+ Diag(Loc, diag::warn_comma_operator);
+ Diag(LHS->getLocStart(), diag::note_cast_to_void)
+ << LHS->getSourceRange()
+ << FixItHint::CreateInsertion(LHS->getLocStart(),
+ LangOpts.CPlusPlus ? "static_cast<void>("
+ : "(void)(")
+ << FixItHint::CreateInsertion(PP.getLocForEndOfToken(LHS->getLocEnd()),
+ ")");
+}
+
// C99 6.5.17
static QualType CheckCommaOperands(Sema &S, ExprResult &LHS, ExprResult &RHS,
SourceLocation Loc) {
@@ -9896,6 +9957,9 @@ static QualType CheckCommaOperands(Sema
diag::err_incomplete_type);
}
+ if (!S.getDiagnostics().isIgnored(diag::warn_comma_operator, Loc))
+ S.DiagnoseCommaOperator(LHS.get(), Loc);
+
return RHS.get()->getType();
}
Modified: cfe/trunk/lib/Sema/SemaStmt.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaStmt.cpp?rev=261278&r1=261277&r2=261278&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaStmt.cpp (original)
+++ cfe/trunk/lib/Sema/SemaStmt.cpp Thu Feb 18 17:58:40 2016
@@ -488,6 +488,20 @@ StmtResult Sema::ActOnAttributedStmt(Sou
return LS;
}
+namespace {
+class CommaVisitor : public EvaluatedExprVisitor<CommaVisitor> {
+ typedef EvaluatedExprVisitor<CommaVisitor> Inherited;
+ Sema &SemaRef;
+public:
+ CommaVisitor(Sema &SemaRef) : Inherited(SemaRef.Context), SemaRef(SemaRef) {}
+ void VisitBinaryOperator(BinaryOperator *E) {
+ if (E->getOpcode() == BO_Comma)
+ SemaRef.DiagnoseCommaOperator(E->getLHS(), E->getExprLoc());
+ EvaluatedExprVisitor<CommaVisitor>::VisitBinaryOperator(E);
+ }
+};
+}
+
StmtResult
Sema::ActOnIfStmt(SourceLocation IfLoc, FullExprArg CondVal, Decl *CondVar,
Stmt *thenStmt, SourceLocation ElseLoc,
@@ -502,6 +516,11 @@ Sema::ActOnIfStmt(SourceLocation IfLoc,
}
Expr *ConditionExpr = CondResult.getAs<Expr>();
if (ConditionExpr) {
+
+ if (!Diags.isIgnored(diag::warn_comma_operator,
+ ConditionExpr->getExprLoc()))
+ CommaVisitor(*this).Visit(ConditionExpr);
+
DiagnoseUnusedExprResult(thenStmt);
if (!elseStmt) {
@@ -1240,6 +1259,10 @@ Sema::ActOnWhileStmt(SourceLocation Whil
return StmtError();
CheckBreakContinueBinding(ConditionExpr);
+ if (ConditionExpr &&
+ !Diags.isIgnored(diag::warn_comma_operator, ConditionExpr->getExprLoc()))
+ CommaVisitor(*this).Visit(ConditionExpr);
+
DiagnoseUnusedExprResult(Body);
if (isa<NullStmt>(Body))
@@ -1642,6 +1665,11 @@ Sema::ActOnForStmt(SourceLocation ForLoc
return StmtError();
}
+ if (SecondResult.get() &&
+ !Diags.isIgnored(diag::warn_comma_operator,
+ SecondResult.get()->getExprLoc()))
+ CommaVisitor(*this).Visit(SecondResult.get());
+
Expr *Third = third.release().getAs<Expr>();
DiagnoseUnusedExprResult(First);
Added: cfe/trunk/test/SemaCXX/warn-comma-operator.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-comma-operator.cpp?rev=261278&view=auto
==============================================================================
--- cfe/trunk/test/SemaCXX/warn-comma-operator.cpp (added)
+++ cfe/trunk/test/SemaCXX/warn-comma-operator.cpp Thu Feb 18 17:58:40 2016
@@ -0,0 +1,278 @@
+// RUN: %clang_cc1 -fsyntax-only -Wcomma -std=c++11 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wcomma -std=c++11 -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+// Test builtin operators
+void test1() {
+ int x = 0, y = 0;
+ for (; y < 10; x++, y++) {}
+ for (; y < 10; ++x, y++) {}
+ for (; y < 10; x++, ++y) {}
+ for (; y < 10; ++x, ++y) {}
+ for (; y < 10; x--, ++y) {}
+ for (; y < 10; --x, ++y) {}
+ for (; y < 10; x = 5, ++y) {}
+ for (; y < 10; x *= 5, ++y) {}
+ for (; y < 10; x /= 5, ++y) {}
+ for (; y < 10; x %= 5, ++y) {}
+ for (; y < 10; x += 5, ++y) {}
+ for (; y < 10; x -= 5, ++y) {}
+ for (; y < 10; x <<= 5, ++y) {}
+ for (; y < 10; x >>= 5, ++y) {}
+ for (; y < 10; x &= 5, ++y) {}
+ for (; y < 10; x |= 5, ++y) {}
+ for (; y < 10; x ^= 5, ++y) {}
+}
+
+class S2 {
+public:
+ void advance();
+
+ S2 operator++();
+ S2 operator++(int);
+ S2 operator--();
+ S2 operator--(int);
+ S2 operator=(int);
+ S2 operator*=(int);
+ S2 operator/=(int);
+ S2 operator%=(int);
+ S2 operator+=(int);
+ S2 operator-=(int);
+ S2 operator<<=(int);
+ S2 operator>>=(int);
+ S2 operator&=(int);
+ S2 operator|=(int);
+ S2 operator^=(int);
+};
+
+// Test overloaded operators
+void test2() {
+ S2 x;
+ int y;
+ for (; y < 10; x++, y++) {}
+ for (; y < 10; ++x, y++) {}
+ for (; y < 10; x++, ++y) {}
+ for (; y < 10; ++x, ++y) {}
+ for (; y < 10; x--, ++y) {}
+ for (; y < 10; --x, ++y) {}
+ for (; y < 10; x = 5, ++y) {}
+ for (; y < 10; x *= 5, ++y) {}
+ for (; y < 10; x /= 5, ++y) {}
+ for (; y < 10; x %= 5, ++y) {}
+ for (; y < 10; x += 5, ++y) {}
+ for (; y < 10; x -= 5, ++y) {}
+ for (; y < 10; x <<= 5, ++y) {}
+ for (; y < 10; x >>= 5, ++y) {}
+ for (; y < 10; x &= 5, ++y) {}
+ for (; y < 10; x |= 5, ++y) {}
+ for (; y < 10; x ^= 5, ++y) {}
+}
+
+// Test nested comma operators
+void test3() {
+ int x1, x2, x3;
+ int y1, *y2 = 0, y3 = 5;
+ for (int z1 = 5, z2 = 4, z3 = 3; x1 <4; ++x1) {}
+}
+
+class Stream {
+ public:
+ Stream& operator<<(int);
+} cout;
+
+int return_four() { return 5; }
+
+// Confusing "," for "<<"
+void test4() {
+ cout << 5 << return_four();
+ cout << 5, return_four();
+ // expected-warning at -1{{comma operator}}
+ // expected-note at -2{{cast expression to void}}
+ // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:3-[[@LINE-3]]:3}:"static_cast<void>("
+ // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:12-[[@LINE-4]]:12}:")"
+}
+
+// Confusing "," for "=="
+void test5() {
+ if (return_four(), 5) {}
+ // expected-warning at -1{{comma operator}}
+ // expected-note at -2{{cast expression to void}}
+ // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:7-[[@LINE-3]]:7}:"static_cast<void>("
+ // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:20-[[@LINE-4]]:20}:")"
+
+ if (return_four() == 5) {}
+}
+
+// Confusing "," for "+"
+int test6() {
+ return return_four(), return_four();
+ // expected-warning at -1{{comma operator}}
+ // expected-note at -2{{cast expression to void}}
+ // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:10-[[@LINE-3]]:10}:"static_cast<void>("
+ // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:23-[[@LINE-4]]:23}:")"
+
+ return return_four() + return_four();
+}
+
+void Concat(int);
+void Concat(int, int);
+
+// Testing extra parentheses in function call
+void test7() {
+ Concat((return_four() , 5));
+ // expected-warning at -1{{comma operator}}
+ // expected-note at -2{{cast expression to void}}
+ // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:11-[[@LINE-3]]:11}:"static_cast<void>("
+ // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:24-[[@LINE-4]]:24}:")"
+
+ Concat(return_four() , 5);
+}
+
+// Be sure to look through parentheses
+void test8() {
+ int x, y;
+ for (x = 0; return_four(), x;) {}
+ // expected-warning at -1{{comma operator}}
+ // expected-note at -2{{cast expression to void}}
+ // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:15-[[@LINE-3]]:15}:"static_cast<void>("
+ // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:28-[[@LINE-4]]:28}:")"
+
+ for (x = 0; (return_four()), (x) ;) {}
+ // expected-warning at -1{{comma operator}}
+ // expected-note at -2{{cast expression to void}}
+ // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:15-[[@LINE-3]]:15}:"static_cast<void>("
+ // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:30-[[@LINE-4]]:30}:")"
+}
+
+bool DoStuff();
+class S9 {
+public:
+ bool Advance();
+ bool More();
+};
+
+// Ignore comma operator in for-loop initializations and increments.
+void test9() {
+ int x, y;
+ for (x = 0, y = 5; x < y; ++x) {}
+ for (x = 0; x < 10; DoStuff(), ++x) {}
+ for (S9 s; s.More(); s.Advance(), ++x) {}
+}
+
+void test10() {
+ int x, y;
+ ++x, ++y;
+ // expected-warning at -1{{comma operator}}
+ // expected-note at -2{{cast expression to void}}
+ // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:3-[[@LINE-3]]:3}:"static_cast<void>("
+ // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:6-[[@LINE-4]]:6}:")"
+}
+
+// Ignore comma operator in templates.
+namespace test11 {
+template <bool T>
+struct B { static const bool value = T; };
+
+typedef B<true> true_type;
+typedef B<false> false_type;
+
+template <bool...>
+struct bool_seq;
+
+template <typename... xs>
+class Foo {
+ typedef bool_seq<(xs::value, true)...> all_true;
+ typedef bool_seq<(xs::value, false)...> all_false;
+ typedef bool_seq<xs::value...> seq;
+};
+
+const auto X = Foo<true_type>();
+}
+
+namespace test12 {
+class Mutex {
+ public:
+ Mutex();
+ ~Mutex();
+};
+class MutexLock {
+public:
+ MutexLock(Mutex &);
+ MutexLock();
+ ~MutexLock();
+};
+class BuiltinMutex {
+ Mutex M;
+};
+Mutex StatusMutex;
+bool Status;
+
+bool get_status() {
+ return (MutexLock(StatusMutex), Status);
+ // expected-warning at -1{{comma operator}}
+ // expected-note at -2{{cast expression to void}}
+ // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:11-[[@LINE-3]]:11}:"static_cast<void>("
+ // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:33-[[@LINE-4]]:33}:")"
+ return (MutexLock(), Status);
+ // expected-warning at -1{{comma operator}}
+ // expected-note at -2{{cast expression to void}}
+ // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:11-[[@LINE-3]]:11}:"static_cast<void>("
+ // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:22-[[@LINE-4]]:22}:")"
+ return (BuiltinMutex(), Status);
+ // expected-warning at -1{{comma operator}}
+ // expected-note at -2{{cast expression to void}}
+ // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:11-[[@LINE-3]]:11}:"static_cast<void>("
+ // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:25-[[@LINE-4]]:25}:")"
+}
+}
+
+// Check for comma operator in conditions.
+void test13(int x) {
+ x = (return_four(), x);
+ // expected-warning at -1{{comma operator}}
+ // expected-note at -2{{cast expression to void}}
+ // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:8-[[@LINE-3]]:8}:"static_cast<void>("
+ // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:21-[[@LINE-4]]:21}:")"
+
+ int y = (return_four(), x);
+ // expected-warning at -1{{comma operator}}
+ // expected-note at -2{{cast expression to void}}
+ // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:12-[[@LINE-3]]:12}:"static_cast<void>("
+ // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:25-[[@LINE-4]]:25}:")"
+
+ for (; return_four(), x;) {}
+ // expected-warning at -1{{comma operator}}
+ // expected-note at -2{{cast expression to void}}
+ // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:10-[[@LINE-3]]:10}:"static_cast<void>("
+ // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:23-[[@LINE-4]]:23}:")"
+
+ while (return_four(), x) {}
+ // expected-warning at -1{{comma operator}}
+ // expected-note at -2{{cast expression to void}}
+ // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:10-[[@LINE-3]]:10}:"static_cast<void>("
+ // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:23-[[@LINE-4]]:23}:")"
+
+ if (return_four(), x) {}
+ // expected-warning at -1{{comma operator}}
+ // expected-note at -2{{cast expression to void}}
+ // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:7-[[@LINE-3]]:7}:"static_cast<void>("
+ // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:20-[[@LINE-4]]:20}:")"
+
+ do { } while (return_four(), x);
+ // expected-warning at -1{{comma operator}}
+ // expected-note at -2{{cast expression to void}}
+ // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:17-[[@LINE-3]]:17}:"static_cast<void>("
+ // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:30-[[@LINE-4]]:30}:")"
+}
+
+// Nested comma operator with fix-its.
+void test14() {
+ return_four(), return_four(), return_four(), return_four();
+ // expected-warning at -1 3{{comma operator}}
+ // expected-note at -2 3{{cast expression to void}}
+ // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:3-[[@LINE-3]]:3}:"static_cast<void>("
+ // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:16-[[@LINE-4]]:16}:")"
+ // CHECK: fix-it:{{.*}}:{[[@LINE-5]]:18-[[@LINE-5]]:18}:"static_cast<void>("
+ // CHECK: fix-it:{{.*}}:{[[@LINE-6]]:31-[[@LINE-6]]:31}:")"
+ // CHECK: fix-it:{{.*}}:{[[@LINE-7]]:33-[[@LINE-7]]:33}:"static_cast<void>("
+ // CHECK: fix-it:{{.*}}:{[[@LINE-8]]:46-[[@LINE-8]]:46}:")"
+}
More information about the cfe-commits
mailing list