Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td =================================================================== --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td @@ -74,6 +74,10 @@ "all paths through this function will call itself">, InGroup, DefaultIgnore; +def warn_comma_operator : Warning<"possible misuse of comma operator here">, + InGroup>, 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">; Index: cfe/trunk/include/clang/Sema/Sema.h =================================================================== --- cfe/trunk/include/clang/Sema/Sema.h +++ cfe/trunk/include/clang/Sema/Sema.h @@ -3971,6 +3971,8 @@ 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, Index: cfe/trunk/lib/Sema/SemaExpr.cpp =================================================================== --- cfe/trunk/lib/Sema/SemaExpr.cpp +++ cfe/trunk/lib/Sema/SemaExpr.cpp @@ -9867,6 +9867,67 @@ ? LHSType : LHSType.getUnqualifiedType()); } +// Only ignore explicit casts to void. +static bool IgnoreCommaOperand(const Expr *E) { + E = E->IgnoreParens(); + + if (const CastExpr *CE = dyn_cast(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(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)(") + << 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 @@ diag::err_incomplete_type); } + if (!S.getDiagnostics().isIgnored(diag::warn_comma_operator, Loc)) + S.DiagnoseCommaOperator(LHS.get(), Loc); + return RHS.get()->getType(); } Index: cfe/trunk/lib/Sema/SemaStmt.cpp =================================================================== --- cfe/trunk/lib/Sema/SemaStmt.cpp +++ cfe/trunk/lib/Sema/SemaStmt.cpp @@ -488,6 +488,20 @@ return LS; } +namespace { +class CommaVisitor : public EvaluatedExprVisitor { + typedef EvaluatedExprVisitor 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::VisitBinaryOperator(E); + } +}; +} + StmtResult Sema::ActOnIfStmt(SourceLocation IfLoc, FullExprArg CondVal, Decl *CondVar, Stmt *thenStmt, SourceLocation ElseLoc, @@ -502,6 +516,11 @@ } Expr *ConditionExpr = CondResult.getAs(); if (ConditionExpr) { + + if (!Diags.isIgnored(diag::warn_comma_operator, + ConditionExpr->getExprLoc())) + CommaVisitor(*this).Visit(ConditionExpr); + DiagnoseUnusedExprResult(thenStmt); if (!elseStmt) { @@ -1240,6 +1259,10 @@ return StmtError(); CheckBreakContinueBinding(ConditionExpr); + if (ConditionExpr && + !Diags.isIgnored(diag::warn_comma_operator, ConditionExpr->getExprLoc())) + CommaVisitor(*this).Visit(ConditionExpr); + DiagnoseUnusedExprResult(Body); if (isa(Body)) @@ -1642,6 +1665,11 @@ return StmtError(); } + if (SecondResult.get() && + !Diags.isIgnored(diag::warn_comma_operator, + SecondResult.get()->getExprLoc())) + CommaVisitor(*this).Visit(SecondResult.get()); + Expr *Third = third.release().getAs(); DiagnoseUnusedExprResult(First); Index: cfe/trunk/test/SemaCXX/warn-comma-operator.cpp =================================================================== --- cfe/trunk/test/SemaCXX/warn-comma-operator.cpp +++ cfe/trunk/test/SemaCXX/warn-comma-operator.cpp @@ -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@-1{{comma operator}} + // expected-note@-2{{cast expression to void}} + // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:3-[[@LINE-3]]:3}:"static_cast(" + // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:12-[[@LINE-4]]:12}:")" +} + +// Confusing "," for "==" +void test5() { + if (return_four(), 5) {} + // expected-warning@-1{{comma operator}} + // expected-note@-2{{cast expression to void}} + // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:7-[[@LINE-3]]:7}:"static_cast(" + // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:20-[[@LINE-4]]:20}:")" + + if (return_four() == 5) {} +} + +// Confusing "," for "+" +int test6() { + return return_four(), return_four(); + // expected-warning@-1{{comma operator}} + // expected-note@-2{{cast expression to void}} + // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:10-[[@LINE-3]]:10}:"static_cast(" + // 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@-1{{comma operator}} + // expected-note@-2{{cast expression to void}} + // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:11-[[@LINE-3]]:11}:"static_cast(" + // 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@-1{{comma operator}} + // expected-note@-2{{cast expression to void}} + // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:15-[[@LINE-3]]:15}:"static_cast(" + // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:28-[[@LINE-4]]:28}:")" + + for (x = 0; (return_four()), (x) ;) {} + // expected-warning@-1{{comma operator}} + // expected-note@-2{{cast expression to void}} + // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:15-[[@LINE-3]]:15}:"static_cast(" + // 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@-1{{comma operator}} + // expected-note@-2{{cast expression to void}} + // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:3-[[@LINE-3]]:3}:"static_cast(" + // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:6-[[@LINE-4]]:6}:")" +} + +// Ignore comma operator in templates. +namespace test11 { +template +struct B { static const bool value = T; }; + +typedef B true_type; +typedef B false_type; + +template +struct bool_seq; + +template +class Foo { + typedef bool_seq<(xs::value, true)...> all_true; + typedef bool_seq<(xs::value, false)...> all_false; + typedef bool_seq seq; +}; + +const auto X = Foo(); +} + +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@-1{{comma operator}} + // expected-note@-2{{cast expression to void}} + // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:11-[[@LINE-3]]:11}:"static_cast(" + // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:33-[[@LINE-4]]:33}:")" + return (MutexLock(), Status); + // expected-warning@-1{{comma operator}} + // expected-note@-2{{cast expression to void}} + // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:11-[[@LINE-3]]:11}:"static_cast(" + // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:22-[[@LINE-4]]:22}:")" + return (BuiltinMutex(), Status); + // expected-warning@-1{{comma operator}} + // expected-note@-2{{cast expression to void}} + // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:11-[[@LINE-3]]:11}:"static_cast(" + // 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@-1{{comma operator}} + // expected-note@-2{{cast expression to void}} + // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:8-[[@LINE-3]]:8}:"static_cast(" + // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:21-[[@LINE-4]]:21}:")" + + int y = (return_four(), x); + // expected-warning@-1{{comma operator}} + // expected-note@-2{{cast expression to void}} + // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:12-[[@LINE-3]]:12}:"static_cast(" + // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:25-[[@LINE-4]]:25}:")" + + for (; return_four(), x;) {} + // expected-warning@-1{{comma operator}} + // expected-note@-2{{cast expression to void}} + // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:10-[[@LINE-3]]:10}:"static_cast(" + // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:23-[[@LINE-4]]:23}:")" + + while (return_four(), x) {} + // expected-warning@-1{{comma operator}} + // expected-note@-2{{cast expression to void}} + // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:10-[[@LINE-3]]:10}:"static_cast(" + // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:23-[[@LINE-4]]:23}:")" + + if (return_four(), x) {} + // expected-warning@-1{{comma operator}} + // expected-note@-2{{cast expression to void}} + // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:7-[[@LINE-3]]:7}:"static_cast(" + // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:20-[[@LINE-4]]:20}:")" + + do { } while (return_four(), x); + // expected-warning@-1{{comma operator}} + // expected-note@-2{{cast expression to void}} + // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:17-[[@LINE-3]]:17}:"static_cast(" + // 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@-1 3{{comma operator}} + // expected-note@-2 3{{cast expression to void}} + // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:3-[[@LINE-3]]:3}:"static_cast(" + // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:16-[[@LINE-4]]:16}:")" + // CHECK: fix-it:{{.*}}:{[[@LINE-5]]:18-[[@LINE-5]]:18}:"static_cast(" + // CHECK: fix-it:{{.*}}:{[[@LINE-6]]:31-[[@LINE-6]]:31}:")" + // CHECK: fix-it:{{.*}}:{[[@LINE-7]]:33-[[@LINE-7]]:33}:"static_cast(" + // CHECK: fix-it:{{.*}}:{[[@LINE-8]]:46-[[@LINE-8]]:46}:")" +}