Index: include/clang/Basic/DiagnosticSemaKinds.td =================================================================== --- include/clang/Basic/DiagnosticSemaKinds.td +++ include/clang/Basic/DiagnosticSemaKinds.td @@ -54,6 +54,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: include/clang/Sema/Sema.h =================================================================== --- include/clang/Sema/Sema.h +++ include/clang/Sema/Sema.h @@ -3636,6 +3636,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: lib/Sema/SemaExpr.cpp =================================================================== --- lib/Sema/SemaExpr.cpp +++ lib/Sema/SemaExpr.cpp @@ -8728,6 +8728,136 @@ ? LHSType : LHSType.getUnqualifiedType()); } +// A whitelist of expressions on the left hand side of a comma operator that +// will prevent it from emitting an error. +static bool IgnoreCommaOperand(const Expr *E) { + E = E->IgnoreParens(); + + // Allow casts to void to silence the warning. + if (const CastExpr *CE = dyn_cast(E)) { + if (CE->getCastKind() == CK_ToVoid) { + return true; + } + } + + // Allow the creation of temporaries that has a cleanup after the RHS is + // evaluated. This effectively wraps the RHS between the LHS construction + // and destruction, such as during RAII use. + if (isa(E->IgnoreCasts())) { + return true; + } + + // Allow assignment and compound assignments. + if (const BinaryOperator *BO = dyn_cast(E)) { + switch (BO->getOpcode()) { + case BO_Assign: + case BO_MulAssign: + case BO_DivAssign: + case BO_RemAssign: + case BO_AddAssign: + case BO_SubAssign: + case BO_ShlAssign: + case BO_ShrAssign: + case BO_AndAssign: + case BO_XorAssign: + case BO_OrAssign: + return true; + default: + return false; + } + } + + // Increments and decrements are commonly chained together into one line. + if (const UnaryOperator *UO = dyn_cast(E)) { + switch (UO->getOpcode()) { + case UO_PostInc: + case UO_PostDec: + case UO_PreInc: + case UO_PreDec: + return true; + default: + return false; + } + } + + if (const CallExpr *CE = dyn_cast(E)) { + if (const FunctionDecl *FD = CE->getDirectCallee()) { + // Unlikely to have a void value on the left hand side of other operators. + if (FD->getReturnType()->isVoidType()) { + return true; + } + + // These are the overloaded versions of the operators from UnaryOperator + // and BinaryOperator above. + switch (FD->getOverloadedOperator()) { + case OO_PlusPlus: + case OO_MinusMinus: + case OO_Equal: + case OO_PlusEqual: + case OO_MinusEqual: + case OO_StarEqual: + case OO_SlashEqual: + case OO_PercentEqual: + case OO_CaretEqual: + case OO_AmpEqual: + case OO_PipeEqual: + case OO_LessLessEqual: + case OO_GreaterGreaterEqual: + return true; + default: + return false; + } + } + } + + 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 skip only the increment in for + // statements, so this check skips the while condition, for-loop condition, + // and for-loop increment. In SemaStmt, we call back to here to handle the + // two conditions that were skipped. + const unsigned ControlFlags = + Scope::ControlScope | Scope::ContinueScope | Scope::BreakScope; + const unsigned ScopeFlags = getCurScope()->getFlags(); + if ((ScopeFlags & ControlFlags) == ControlFlags) + 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(); + } + + // Whitelist some expressions to assume to be safe for the left hand + // expression + 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) { @@ -8757,6 +8887,9 @@ diag::err_incomplete_type); } + if (!S.getDiagnostics().isIgnored(diag::warn_comma_operator, Loc)) + S.DiagnoseCommaOperator(LHS.get(), Loc); + return RHS.get()->getType(); } Index: lib/Sema/SemaStmt.cpp =================================================================== --- lib/Sema/SemaStmt.cpp +++ lib/Sema/SemaStmt.cpp @@ -1207,6 +1207,20 @@ } } +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::ActOnWhileStmt(SourceLocation WhileLoc, FullExprArg Cond, Decl *CondVar, Stmt *Body) { @@ -1224,6 +1238,10 @@ return StmtError(); CheckBreakContinueBinding(ConditionExpr); + if (ConditionExpr && + !Diags.isIgnored(diag::warn_comma_operator, ConditionExpr->getExprLoc())) + CommaVisitor(*this).Visit(ConditionExpr); + DiagnoseUnusedExprResult(Body); if (isa(Body)) @@ -1625,6 +1643,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: test/SemaCXX/warn-comma-operator.cpp =================================================================== --- test/SemaCXX/warn-comma-operator.cpp +++ test/SemaCXX/warn-comma-operator.cpp @@ -0,0 +1,293 @@ +// 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) {} + + x++, ++x, x++, ++x, x--, --x, ++y; + x = 5, x *= 5, x /= 5, x %= 5, x += 5,x -= 5, ++y; + x <<= 5, x >>= 5, x &= 5, x |= 5, 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) {} + + x++, ++x, x++, ++x, x--, --x, ++y; + x = 5, x *= 5, x /= 5, x %= 5, x += 5,x -= 5, ++y; + x <<= 5, x >>= 5, x &= 5, x |= 5, x ^= 5, ++y; +} + +// Test nested comma operators +void test3() { + int x1, x2, x3; + int y1, *y2 = 0, y3 = 5; + for (x1 = 5, x2 = 4, x3 = 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}:")" + + for ((x = 0), (y = 0); x < y; (x++), (y++)) {} + for (x = 0, y = 0; x < y; x++, y++) {} +} + +bool DoStuff(); +class S9 { +public: + bool Advance(); + bool More(); +}; + +// Ignore comma operator in for-loop increments. +void test9() { + int x; + for (x = 0; x < 10; DoStuff(), ++x) {} + for (S9 s; s.More(); s.Advance(), ++x) {} +} + +// Allow cast to void to silence warning +void test10() { + if (return_four(), true) {} + // 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 ((void)return_four(), true) {} + if (static_cast(return_four()), true) {} +} + +// 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; + +// Allow RAII objects in the LHS. +bool get_status() { + return (MutexLock(StatusMutex), Status); + return (MutexLock(), Status); + return (BuiltinMutex(), Status); +} +} + +// 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}:")" +} + +void return_void() { return; } + +// Ignore when the LHS is a void returning function. +void test14(int x) { + x = (return_void(), x); + int y = (return_void(), x); + for (; return_void(), x;) {} + while (return_void(), x) {} + if (return_void(), x) {} + do { } while (return_void(), x); + return_void(), return_void(), return_four(); +} + +// Nested comma operator with fix-its. +void test15() { + 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}:")" +}