Skip to content

Commit faca2d8

Browse files
WeverythingWeverything
committedFeb 18, 2016
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 llvm-svn: 261278
·
llvmorg-21-initllvmorg-3.9.0-rc1
1 parent 0adbea4 commit faca2d8

File tree

5 files changed

+376
-0
lines changed

5 files changed

+376
-0
lines changed
 

‎clang/include/clang/Basic/DiagnosticSemaKinds.td

+4
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,10 @@ def warn_infinite_recursive_function : Warning<
7474
"all paths through this function will call itself">,
7575
InGroup<InfiniteRecursion>, DefaultIgnore;
7676

77+
def warn_comma_operator : Warning<"possible misuse of comma operator here">,
78+
InGroup<DiagGroup<"comma">>, DefaultIgnore;
79+
def note_cast_to_void : Note<"cast expression to void to silence warning">;
80+
7781
// Constant expressions
7882
def err_expr_not_ice : Error<
7983
"expression is not an %select{integer|integral}0 constant expression">;

‎clang/include/clang/Sema/Sema.h

+2
Original file line numberDiff line numberDiff line change
@@ -3971,6 +3971,8 @@ class Sema {
39713971
ExprResult CreateBuiltinBinOp(SourceLocation OpLoc, BinaryOperatorKind Opc,
39723972
Expr *LHSExpr, Expr *RHSExpr);
39733973

3974+
void DiagnoseCommaOperator(const Expr *LHS, SourceLocation Loc);
3975+
39743976
/// ActOnConditionalOp - Parse a ?: operation. Note that 'LHS' may be null
39753977
/// in the case of a the GNU conditional expr extension.
39763978
ExprResult ActOnConditionalOp(SourceLocation QuestionLoc,

‎clang/lib/Sema/SemaExpr.cpp

+64
Original file line numberDiff line numberDiff line change
@@ -9867,6 +9867,67 @@ QualType Sema::CheckAssignmentOperands(Expr *LHSExpr, ExprResult &RHS,
98679867
? LHSType : LHSType.getUnqualifiedType());
98689868
}
98699869

9870+
// Only ignore explicit casts to void.
9871+
static bool IgnoreCommaOperand(const Expr *E) {
9872+
E = E->IgnoreParens();
9873+
9874+
if (const CastExpr *CE = dyn_cast<CastExpr>(E)) {
9875+
if (CE->getCastKind() == CK_ToVoid) {
9876+
return true;
9877+
}
9878+
}
9879+
9880+
return false;
9881+
}
9882+
9883+
// Look for instances where it is likely the comma operator is confused with
9884+
// another operator. There is a whitelist of acceptable expressions for the
9885+
// left hand side of the comma operator, otherwise emit a warning.
9886+
void Sema::DiagnoseCommaOperator(const Expr *LHS, SourceLocation Loc) {
9887+
// No warnings in macros
9888+
if (Loc.isMacroID())
9889+
return;
9890+
9891+
// Don't warn in template instantiations.
9892+
if (!ActiveTemplateInstantiations.empty())
9893+
return;
9894+
9895+
// Scope isn't fine-grained enough to whitelist the specific cases, so
9896+
// instead, skip more than needed, then call back into here with the
9897+
// CommaVisitor in SemaStmt.cpp.
9898+
// The whitelisted locations are the initialization and increment portions
9899+
// of a for loop. The additional checks are on the condition of
9900+
// if statements, do/while loops, and for loops.
9901+
const unsigned ForIncreamentFlags =
9902+
Scope::ControlScope | Scope::ContinueScope | Scope::BreakScope;
9903+
const unsigned ForInitFlags = Scope::ControlScope | Scope::DeclScope;
9904+
const unsigned ScopeFlags = getCurScope()->getFlags();
9905+
if ((ScopeFlags & ForIncrementFlags) == ForIncrementFlags ||
9906+
(ScopeFlags & ForInitFlags) == ForInitFlags)
9907+
return;
9908+
9909+
// If there are multiple comma operators used together, get the RHS of the
9910+
// of the comma operator as the LHS.
9911+
while (const BinaryOperator *BO = dyn_cast<BinaryOperator>(LHS)) {
9912+
if (BO->getOpcode() != BO_Comma)
9913+
break;
9914+
LHS = BO->getRHS();
9915+
}
9916+
9917+
// Only allow some expressions on LHS to not warn.
9918+
if (IgnoreCommaOperand(LHS))
9919+
return;
9920+
9921+
Diag(Loc, diag::warn_comma_operator);
9922+
Diag(LHS->getLocStart(), diag::note_cast_to_void)
9923+
<< LHS->getSourceRange()
9924+
<< FixItHint::CreateInsertion(LHS->getLocStart(),
9925+
LangOpts.CPlusPlus ? "static_cast<void>("
9926+
: "(void)(")
9927+
<< FixItHint::CreateInsertion(PP.getLocForEndOfToken(LHS->getLocEnd()),
9928+
")");
9929+
}
9930+
98709931
// C99 6.5.17
98719932
static QualType CheckCommaOperands(Sema &S, ExprResult &LHS, ExprResult &RHS,
98729933
SourceLocation Loc) {
@@ -9896,6 +9957,9 @@ static QualType CheckCommaOperands(Sema &S, ExprResult &LHS, ExprResult &RHS,
98969957
diag::err_incomplete_type);
98979958
}
98989959

9960+
if (!S.getDiagnostics().isIgnored(diag::warn_comma_operator, Loc))
9961+
S.DiagnoseCommaOperator(LHS.get(), Loc);
9962+
98999963
return RHS.get()->getType();
99009964
}
99019965

‎clang/lib/Sema/SemaStmt.cpp

+28
Original file line numberDiff line numberDiff line change
@@ -488,6 +488,20 @@ StmtResult Sema::ActOnAttributedStmt(SourceLocation AttrLoc,
488488
return LS;
489489
}
490490

491+
namespace {
492+
class CommaVisitor : public EvaluatedExprVisitor<CommaVisitor> {
493+
typedef EvaluatedExprVisitor<CommaVisitor> Inherited;
494+
Sema &SemaRef;
495+
public:
496+
CommaVisitor(Sema &SemaRef) : Inherited(SemaRef.Context), SemaRef(SemaRef) {}
497+
void VisitBinaryOperator(BinaryOperator *E) {
498+
if (E->getOpcode() == BO_Comma)
499+
SemaRef.DiagnoseCommaOperator(E->getLHS(), E->getExprLoc());
500+
EvaluatedExprVisitor<CommaVisitor>::VisitBinaryOperator(E);
501+
}
502+
};
503+
}
504+
491505
StmtResult
492506
Sema::ActOnIfStmt(SourceLocation IfLoc, FullExprArg CondVal, Decl *CondVar,
493507
Stmt *thenStmt, SourceLocation ElseLoc,
@@ -502,6 +516,11 @@ Sema::ActOnIfStmt(SourceLocation IfLoc, FullExprArg CondVal, Decl *CondVar,
502516
}
503517
Expr *ConditionExpr = CondResult.getAs<Expr>();
504518
if (ConditionExpr) {
519+
520+
if (!Diags.isIgnored(diag::warn_comma_operator,
521+
ConditionExpr->getExprLoc()))
522+
CommaVisitor(*this).Visit(ConditionExpr);
523+
505524
DiagnoseUnusedExprResult(thenStmt);
506525

507526
if (!elseStmt) {
@@ -1240,6 +1259,10 @@ Sema::ActOnWhileStmt(SourceLocation WhileLoc, FullExprArg Cond,
12401259
return StmtError();
12411260
CheckBreakContinueBinding(ConditionExpr);
12421261

1262+
if (ConditionExpr &&
1263+
!Diags.isIgnored(diag::warn_comma_operator, ConditionExpr->getExprLoc()))
1264+
CommaVisitor(*this).Visit(ConditionExpr);
1265+
12431266
DiagnoseUnusedExprResult(Body);
12441267

12451268
if (isa<NullStmt>(Body))
@@ -1642,6 +1665,11 @@ Sema::ActOnForStmt(SourceLocation ForLoc, SourceLocation LParenLoc,
16421665
return StmtError();
16431666
}
16441667

1668+
if (SecondResult.get() &&
1669+
!Diags.isIgnored(diag::warn_comma_operator,
1670+
SecondResult.get()->getExprLoc()))
1671+
CommaVisitor(*this).Visit(SecondResult.get());
1672+
16451673
Expr *Third = third.release().getAs<Expr>();
16461674

16471675
DiagnoseUnusedExprResult(First);

‎clang/test/SemaCXX/warn-comma-operator.cpp

+278
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,278 @@
1+
// RUN: %clang_cc1 -fsyntax-only -Wcomma -std=c++11 -verify %s
2+
// RUN: %clang_cc1 -fsyntax-only -Wcomma -std=c++11 -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
3+
4+
// Test builtin operators
5+
void test1() {
6+
int x = 0, y = 0;
7+
for (; y < 10; x++, y++) {}
8+
for (; y < 10; ++x, y++) {}
9+
for (; y < 10; x++, ++y) {}
10+
for (; y < 10; ++x, ++y) {}
11+
for (; y < 10; x--, ++y) {}
12+
for (; y < 10; --x, ++y) {}
13+
for (; y < 10; x = 5, ++y) {}
14+
for (; y < 10; x *= 5, ++y) {}
15+
for (; y < 10; x /= 5, ++y) {}
16+
for (; y < 10; x %= 5, ++y) {}
17+
for (; y < 10; x += 5, ++y) {}
18+
for (; y < 10; x -= 5, ++y) {}
19+
for (; y < 10; x <<= 5, ++y) {}
20+
for (; y < 10; x >>= 5, ++y) {}
21+
for (; y < 10; x &= 5, ++y) {}
22+
for (; y < 10; x |= 5, ++y) {}
23+
for (; y < 10; x ^= 5, ++y) {}
24+
}
25+
26+
class S2 {
27+
public:
28+
void advance();
29+
30+
S2 operator++();
31+
S2 operator++(int);
32+
S2 operator--();
33+
S2 operator--(int);
34+
S2 operator=(int);
35+
S2 operator*=(int);
36+
S2 operator/=(int);
37+
S2 operator%=(int);
38+
S2 operator+=(int);
39+
S2 operator-=(int);
40+
S2 operator<<=(int);
41+
S2 operator>>=(int);
42+
S2 operator&=(int);
43+
S2 operator|=(int);
44+
S2 operator^=(int);
45+
};
46+
47+
// Test overloaded operators
48+
void test2() {
49+
S2 x;
50+
int y;
51+
for (; y < 10; x++, y++) {}
52+
for (; y < 10; ++x, y++) {}
53+
for (; y < 10; x++, ++y) {}
54+
for (; y < 10; ++x, ++y) {}
55+
for (; y < 10; x--, ++y) {}
56+
for (; y < 10; --x, ++y) {}
57+
for (; y < 10; x = 5, ++y) {}
58+
for (; y < 10; x *= 5, ++y) {}
59+
for (; y < 10; x /= 5, ++y) {}
60+
for (; y < 10; x %= 5, ++y) {}
61+
for (; y < 10; x += 5, ++y) {}
62+
for (; y < 10; x -= 5, ++y) {}
63+
for (; y < 10; x <<= 5, ++y) {}
64+
for (; y < 10; x >>= 5, ++y) {}
65+
for (; y < 10; x &= 5, ++y) {}
66+
for (; y < 10; x |= 5, ++y) {}
67+
for (; y < 10; x ^= 5, ++y) {}
68+
}
69+
70+
// Test nested comma operators
71+
void test3() {
72+
int x1, x2, x3;
73+
int y1, *y2 = 0, y3 = 5;
74+
for (int z1 = 5, z2 = 4, z3 = 3; x1 <4; ++x1) {}
75+
}
76+
77+
class Stream {
78+
public:
79+
Stream& operator<<(int);
80+
} cout;
81+
82+
int return_four() { return 5; }
83+
84+
// Confusing "," for "<<"
85+
void test4() {
86+
cout << 5 << return_four();
87+
cout << 5, return_four();
88+
// expected-warning@-1{{comma operator}}
89+
// expected-note@-2{{cast expression to void}}
90+
// CHECK: fix-it:{{.*}}:{[[@LINE-3]]:3-[[@LINE-3]]:3}:"static_cast<void>("
91+
// CHECK: fix-it:{{.*}}:{[[@LINE-4]]:12-[[@LINE-4]]:12}:")"
92+
}
93+
94+
// Confusing "," for "=="
95+
void test5() {
96+
if (return_four(), 5) {}
97+
// expected-warning@-1{{comma operator}}
98+
// expected-note@-2{{cast expression to void}}
99+
// CHECK: fix-it:{{.*}}:{[[@LINE-3]]:7-[[@LINE-3]]:7}:"static_cast<void>("
100+
// CHECK: fix-it:{{.*}}:{[[@LINE-4]]:20-[[@LINE-4]]:20}:")"
101+
102+
if (return_four() == 5) {}
103+
}
104+
105+
// Confusing "," for "+"
106+
int test6() {
107+
return return_four(), return_four();
108+
// expected-warning@-1{{comma operator}}
109+
// expected-note@-2{{cast expression to void}}
110+
// CHECK: fix-it:{{.*}}:{[[@LINE-3]]:10-[[@LINE-3]]:10}:"static_cast<void>("
111+
// CHECK: fix-it:{{.*}}:{[[@LINE-4]]:23-[[@LINE-4]]:23}:")"
112+
113+
return return_four() + return_four();
114+
}
115+
116+
void Concat(int);
117+
void Concat(int, int);
118+
119+
// Testing extra parentheses in function call
120+
void test7() {
121+
Concat((return_four() , 5));
122+
// expected-warning@-1{{comma operator}}
123+
// expected-note@-2{{cast expression to void}}
124+
// CHECK: fix-it:{{.*}}:{[[@LINE-3]]:11-[[@LINE-3]]:11}:"static_cast<void>("
125+
// CHECK: fix-it:{{.*}}:{[[@LINE-4]]:24-[[@LINE-4]]:24}:")"
126+
127+
Concat(return_four() , 5);
128+
}
129+
130+
// Be sure to look through parentheses
131+
void test8() {
132+
int x, y;
133+
for (x = 0; return_four(), x;) {}
134+
// expected-warning@-1{{comma operator}}
135+
// expected-note@-2{{cast expression to void}}
136+
// CHECK: fix-it:{{.*}}:{[[@LINE-3]]:15-[[@LINE-3]]:15}:"static_cast<void>("
137+
// CHECK: fix-it:{{.*}}:{[[@LINE-4]]:28-[[@LINE-4]]:28}:")"
138+
139+
for (x = 0; (return_four()), (x) ;) {}
140+
// expected-warning@-1{{comma operator}}
141+
// expected-note@-2{{cast expression to void}}
142+
// CHECK: fix-it:{{.*}}:{[[@LINE-3]]:15-[[@LINE-3]]:15}:"static_cast<void>("
143+
// CHECK: fix-it:{{.*}}:{[[@LINE-4]]:30-[[@LINE-4]]:30}:")"
144+
}
145+
146+
bool DoStuff();
147+
class S9 {
148+
public:
149+
bool Advance();
150+
bool More();
151+
};
152+
153+
// Ignore comma operator in for-loop initializations and increments.
154+
void test9() {
155+
int x, y;
156+
for (x = 0, y = 5; x < y; ++x) {}
157+
for (x = 0; x < 10; DoStuff(), ++x) {}
158+
for (S9 s; s.More(); s.Advance(), ++x) {}
159+
}
160+
161+
void test10() {
162+
int x, y;
163+
++x, ++y;
164+
// expected-warning@-1{{comma operator}}
165+
// expected-note@-2{{cast expression to void}}
166+
// CHECK: fix-it:{{.*}}:{[[@LINE-3]]:3-[[@LINE-3]]:3}:"static_cast<void>("
167+
// CHECK: fix-it:{{.*}}:{[[@LINE-4]]:6-[[@LINE-4]]:6}:")"
168+
}
169+
170+
// Ignore comma operator in templates.
171+
namespace test11 {
172+
template <bool T>
173+
struct B { static const bool value = T; };
174+
175+
typedef B<true> true_type;
176+
typedef B<false> false_type;
177+
178+
template <bool...>
179+
struct bool_seq;
180+
181+
template <typename... xs>
182+
class Foo {
183+
typedef bool_seq<(xs::value, true)...> all_true;
184+
typedef bool_seq<(xs::value, false)...> all_false;
185+
typedef bool_seq<xs::value...> seq;
186+
};
187+
188+
const auto X = Foo<true_type>();
189+
}
190+
191+
namespace test12 {
192+
class Mutex {
193+
public:
194+
Mutex();
195+
~Mutex();
196+
};
197+
class MutexLock {
198+
public:
199+
MutexLock(Mutex &);
200+
MutexLock();
201+
~MutexLock();
202+
};
203+
class BuiltinMutex {
204+
Mutex M;
205+
};
206+
Mutex StatusMutex;
207+
bool Status;
208+
209+
bool get_status() {
210+
return (MutexLock(StatusMutex), Status);
211+
// expected-warning@-1{{comma operator}}
212+
// expected-note@-2{{cast expression to void}}
213+
// CHECK: fix-it:{{.*}}:{[[@LINE-3]]:11-[[@LINE-3]]:11}:"static_cast<void>("
214+
// CHECK: fix-it:{{.*}}:{[[@LINE-4]]:33-[[@LINE-4]]:33}:")"
215+
return (MutexLock(), Status);
216+
// expected-warning@-1{{comma operator}}
217+
// expected-note@-2{{cast expression to void}}
218+
// CHECK: fix-it:{{.*}}:{[[@LINE-3]]:11-[[@LINE-3]]:11}:"static_cast<void>("
219+
// CHECK: fix-it:{{.*}}:{[[@LINE-4]]:22-[[@LINE-4]]:22}:")"
220+
return (BuiltinMutex(), Status);
221+
// expected-warning@-1{{comma operator}}
222+
// expected-note@-2{{cast expression to void}}
223+
// CHECK: fix-it:{{.*}}:{[[@LINE-3]]:11-[[@LINE-3]]:11}:"static_cast<void>("
224+
// CHECK: fix-it:{{.*}}:{[[@LINE-4]]:25-[[@LINE-4]]:25}:")"
225+
}
226+
}
227+
228+
// Check for comma operator in conditions.
229+
void test13(int x) {
230+
x = (return_four(), x);
231+
// expected-warning@-1{{comma operator}}
232+
// expected-note@-2{{cast expression to void}}
233+
// CHECK: fix-it:{{.*}}:{[[@LINE-3]]:8-[[@LINE-3]]:8}:"static_cast<void>("
234+
// CHECK: fix-it:{{.*}}:{[[@LINE-4]]:21-[[@LINE-4]]:21}:")"
235+
236+
int y = (return_four(), x);
237+
// expected-warning@-1{{comma operator}}
238+
// expected-note@-2{{cast expression to void}}
239+
// CHECK: fix-it:{{.*}}:{[[@LINE-3]]:12-[[@LINE-3]]:12}:"static_cast<void>("
240+
// CHECK: fix-it:{{.*}}:{[[@LINE-4]]:25-[[@LINE-4]]:25}:")"
241+
242+
for (; return_four(), x;) {}
243+
// expected-warning@-1{{comma operator}}
244+
// expected-note@-2{{cast expression to void}}
245+
// CHECK: fix-it:{{.*}}:{[[@LINE-3]]:10-[[@LINE-3]]:10}:"static_cast<void>("
246+
// CHECK: fix-it:{{.*}}:{[[@LINE-4]]:23-[[@LINE-4]]:23}:")"
247+
248+
while (return_four(), x) {}
249+
// expected-warning@-1{{comma operator}}
250+
// expected-note@-2{{cast expression to void}}
251+
// CHECK: fix-it:{{.*}}:{[[@LINE-3]]:10-[[@LINE-3]]:10}:"static_cast<void>("
252+
// CHECK: fix-it:{{.*}}:{[[@LINE-4]]:23-[[@LINE-4]]:23}:")"
253+
254+
if (return_four(), x) {}
255+
// expected-warning@-1{{comma operator}}
256+
// expected-note@-2{{cast expression to void}}
257+
// CHECK: fix-it:{{.*}}:{[[@LINE-3]]:7-[[@LINE-3]]:7}:"static_cast<void>("
258+
// CHECK: fix-it:{{.*}}:{[[@LINE-4]]:20-[[@LINE-4]]:20}:")"
259+
260+
do { } while (return_four(), x);
261+
// expected-warning@-1{{comma operator}}
262+
// expected-note@-2{{cast expression to void}}
263+
// CHECK: fix-it:{{.*}}:{[[@LINE-3]]:17-[[@LINE-3]]:17}:"static_cast<void>("
264+
// CHECK: fix-it:{{.*}}:{[[@LINE-4]]:30-[[@LINE-4]]:30}:")"
265+
}
266+
267+
// Nested comma operator with fix-its.
268+
void test14() {
269+
return_four(), return_four(), return_four(), return_four();
270+
// expected-warning@-1 3{{comma operator}}
271+
// expected-note@-2 3{{cast expression to void}}
272+
// CHECK: fix-it:{{.*}}:{[[@LINE-3]]:3-[[@LINE-3]]:3}:"static_cast<void>("
273+
// CHECK: fix-it:{{.*}}:{[[@LINE-4]]:16-[[@LINE-4]]:16}:")"
274+
// CHECK: fix-it:{{.*}}:{[[@LINE-5]]:18-[[@LINE-5]]:18}:"static_cast<void>("
275+
// CHECK: fix-it:{{.*}}:{[[@LINE-6]]:31-[[@LINE-6]]:31}:")"
276+
// CHECK: fix-it:{{.*}}:{[[@LINE-7]]:33-[[@LINE-7]]:33}:"static_cast<void>("
277+
// CHECK: fix-it:{{.*}}:{[[@LINE-8]]:46-[[@LINE-8]]:46}:")"
278+
}

0 commit comments

Comments
 (0)
Please sign in to comment.