Add another exemption for RAII objects in the LHS of the comma operator.
http://reviews.llvm.org/D3976
Files:
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Sema/Sema.h
lib/Sema/SemaExpr.cpp
lib/Sema/SemaOverload.cpp
test/SemaCXX/warn-comma-operator.cpp
Index: test/SemaCXX/warn-comma-operator.cpp
===================================================================
--- test/SemaCXX/warn-comma-operator.cpp
+++ test/SemaCXX/warn-comma-operator.cpp
@@ -0,0 +1,227 @@
+// 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 (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<void>("
+ // 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<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@-1{{comma operator}}
+ // expected-note@-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@-1{{comma operator}}
+ // expected-note@-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@-1{{comma operator}}
+ // expected-note@-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@-1{{comma operator}}
+ // expected-note@-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}:")"
+
+ for ((x = 0), (y = 0); x < y; (x++), (y++)) {}
+ for (x = 0, y = 0; x < y; x++, y++) {}
+}
+
+void DoStuff();
+class S9 {
+public:
+ void Advance();
+ bool More();
+};
+// Ignore functions with void return type.
+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<void>("
+ // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:20-[[@LINE-4]]:20}:")"
+
+ if ((void)return_four(), true) {}
+ if (static_cast<void>(return_four()), true) {}
+}
+
+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;
+ // expected-warning@-1{{comma operator}}
+ // expected-note@-2{{cast expression to void}}
+ // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:21-[[@LINE-3]]:21}:"static_cast<void>("
+ // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:30-[[@LINE-4]]:30}:")"
+
+ typedef bool_seq<(xs::value, false)...> all_false;
+ // expected-warning@-1{{comma operator}}
+ // expected-note@-2{{cast expression to void}}
+ // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:21-[[@LINE-3]]:21}:"static_cast<void>("
+ // CHECK: fix-it:{{.*}}:{[[@LINE-4]]:30-[[@LINE-4]]:30}:")"
+
+ 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);
+ return (MutexLock(), Status);
+ return (BuiltinMutex(), Status);
+}
+}
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<InfiniteRecursion>, DefaultIgnore;
+def warn_comma_operator : Warning<"possible misuse of comma operator here">,
+ InGroup<DiagGroup<"comma">>, DefaultIgnore;
+def note_cast_to_null : 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/SemaOverload.cpp
===================================================================
--- lib/Sema/SemaOverload.cpp
+++ lib/Sema/SemaOverload.cpp
@@ -10888,10 +10888,14 @@
if (Fns.empty()) {
// If there are no functions to store, just build a dependent
// BinaryOperator or CompoundAssignment.
- if (Opc <= BO_Assign || Opc > BO_OrAssign)
+ if (Opc <= BO_Assign || Opc > BO_OrAssign) {
+ if (Opc == BO_Comma) {
+ DiagnoseCommaOperator(Args[0], OpLoc);
+ }
return new (Context) BinaryOperator(
Args[0], Args[1], Opc, Context.DependentTy, VK_RValue, OK_Ordinary,
OpLoc, FPFeatures.fp_contract);
+ }
return new (Context) CompoundAssignOperator(
Args[0], Args[1], Opc, Context.DependentTy, VK_LValue, OK_Ordinary,
Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -8728,6 +8728,126 @@
? 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<CastExpr>(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<CXXBindTemporaryExpr>(E->IgnoreCasts())) {
+ return true;
+ }
+
+ // Allow assignment and compound assignments. Also recurse on other
+ // comma operators.
+ if (const BinaryOperator *BO = dyn_cast<BinaryOperator>(E)) {
+ switch (BO->getOpcode()) {
+ case BO_Comma:
+ return IgnoreCommaOperand(BO->getRHS());
+ 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 widely used in for loops.
+ if (const UnaryOperator *UO = dyn_cast<UnaryOperator>(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<CallExpr>(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;
+
+ // Whitelist some expressions to assume to be safe for the left hand
+ // expression
+ if (IgnoreCommaOperand(LHS))
+ return;
+
+ const BinaryOperator *BO = nullptr;
+ while ((BO = dyn_cast<BinaryOperator>(LHS)) && BO->getOpcode() == BO_Comma) {
+ LHS = BO->getRHS();
+ }
+
+ Diag(Loc, diag::warn_comma_operator);
+ Diag(LHS->getLocStart(), diag::note_cast_to_null)
+ << 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) {
@@ -8757,6 +8877,8 @@
diag::err_incomplete_type);
}
+ S.DiagnoseCommaOperator(LHS.get(), Loc);
+
return RHS.get()->getType();
}
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
