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
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index cbac36b..f371407 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/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<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 @@
diag::err_incomplete_type);
}
+ if (!S.getDiagnostics().isIgnored(diag::warn_comma_operator, Loc))
+ S.DiagnoseCommaOperator(LHS.get(), Loc);
+
return RHS.get()->getType();
}
diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index 350f16d..c58bf46 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -488,6 +488,20 @@
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 @@
}
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 @@
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 @@
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);