Refactor the way we handle diagnosing unused expression results.
Rather than sprinkle calls to DiagnoseUnusedExprResult() around in places where we want diagnostics, we now diagnose unused expression statements and full expressions in a more generic way when acting on the final expression statement. This results in more appropriate diagnostics for [[nodiscard]] where we were previously lacking them, such as when the body of a for loop is not a compound statement.
This patch fixes PR39837.
llvm-svn: 350404
diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index dacf8d0..9e30c9a 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -42,12 +42,11 @@
using namespace clang;
using namespace sema;
-StmtResult Sema::ActOnExprStmt(ExprResult FE) {
+StmtResult Sema::ActOnExprStmt(ExprResult FE, bool DiscardedValue) {
if (FE.isInvalid())
return StmtError();
- FE = ActOnFinishFullExpr(FE.get(), FE.get()->getExprLoc(),
- /*DiscardedValue*/ true);
+ FE = ActOnFinishFullExpr(FE.get(), FE.get()->getExprLoc(), DiscardedValue);
if (FE.isInvalid())
return StmtError();
@@ -348,6 +347,10 @@
return getCurFunction()->CompoundScopes.back();
}
+bool Sema::isCurCompoundStmtAStmtExpr() const {
+ return getCurCompoundScope().IsStmtExpr;
+}
+
StmtResult Sema::ActOnCompoundStmt(SourceLocation L, SourceLocation R,
ArrayRef<Stmt *> Elts, bool isStmtExpr) {
const unsigned NumElts = Elts.size();
@@ -370,14 +373,6 @@
Diag(D->getLocation(), diag::ext_mixed_decls_code);
}
}
- // Warn about unused expressions in statements.
- for (unsigned i = 0; i != NumElts; ++i) {
- // Ignore statements that are last in a statement expression.
- if (isStmtExpr && i == NumElts - 1)
- continue;
-
- DiagnoseUnusedExprResult(Elts[i]);
- }
// Check for suspicious empty body (null statement) in `for' and `while'
// statements. Don't do anything for template instantiations, this just adds
@@ -469,15 +464,12 @@
/// ActOnCaseStmtBody - This installs a statement as the body of a case.
void Sema::ActOnCaseStmtBody(Stmt *S, Stmt *SubStmt) {
- DiagnoseUnusedExprResult(SubStmt);
cast<CaseStmt>(S)->setSubStmt(SubStmt);
}
StmtResult
Sema::ActOnDefaultStmt(SourceLocation DefaultLoc, SourceLocation ColonLoc,
Stmt *SubStmt, Scope *CurScope) {
- DiagnoseUnusedExprResult(SubStmt);
-
if (getCurFunction()->SwitchStack.empty()) {
Diag(DefaultLoc, diag::err_default_not_in_switch);
return SubStmt;
@@ -571,9 +563,6 @@
if (IsConstexpr || isa<ObjCAvailabilityCheckExpr>(Cond.get().second))
setFunctionHasBranchProtectedScope();
- DiagnoseUnusedExprResult(thenStmt);
- DiagnoseUnusedExprResult(elseStmt);
-
return IfStmt::Create(Context, IfLoc, IsConstexpr, InitStmt, Cond.get().first,
Cond.get().second, thenStmt, ElseLoc, elseStmt);
}
@@ -1301,8 +1290,6 @@
!Diags.isIgnored(diag::warn_comma_operator, CondVal.second->getExprLoc()))
CommaVisitor(*this).Visit(CondVal.second);
- DiagnoseUnusedExprResult(Body);
-
if (isa<NullStmt>(Body))
getCurCompoundScope().setHasEmptyLoopBodies();
@@ -1322,7 +1309,7 @@
return StmtError();
Cond = CondResult.get();
- CondResult = ActOnFinishFullExpr(Cond, DoLoc);
+ CondResult = ActOnFinishFullExpr(Cond, DoLoc, /*DiscardedValue*/ false);
if (CondResult.isInvalid())
return StmtError();
Cond = CondResult.get();
@@ -1332,8 +1319,6 @@
!Diags.isIgnored(diag::warn_comma_operator, Cond->getExprLoc()))
CommaVisitor(*this).Visit(Cond);
- DiagnoseUnusedExprResult(Body);
-
return new (Context) DoStmt(Body, Cond, DoLoc, WhileLoc, CondRParen);
}
@@ -1778,11 +1763,6 @@
CommaVisitor(*this).Visit(Second.get().second);
Expr *Third = third.release().getAs<Expr>();
-
- DiagnoseUnusedExprResult(First);
- DiagnoseUnusedExprResult(Third);
- DiagnoseUnusedExprResult(Body);
-
if (isa<NullStmt>(Body))
getCurCompoundScope().setHasEmptyLoopBodies();
@@ -1802,7 +1782,7 @@
if (result.isInvalid()) return StmtError();
E = result.get();
- ExprResult FullExpr = ActOnFinishFullExpr(E);
+ ExprResult FullExpr = ActOnFinishFullExpr(E, /*DiscardedValue*/ false);
if (FullExpr.isInvalid())
return StmtError();
return StmtResult(static_cast<Stmt*>(FullExpr.get()));
@@ -1956,7 +1936,8 @@
if (CollectionExprResult.isInvalid())
return StmtError();
- CollectionExprResult = ActOnFinishFullExpr(CollectionExprResult.get());
+ CollectionExprResult =
+ ActOnFinishFullExpr(CollectionExprResult.get(), /*DiscardedValue*/ false);
if (CollectionExprResult.isInvalid())
return StmtError();
@@ -2593,7 +2574,8 @@
if (!NotEqExpr.isInvalid())
NotEqExpr = CheckBooleanCondition(ColonLoc, NotEqExpr.get());
if (!NotEqExpr.isInvalid())
- NotEqExpr = ActOnFinishFullExpr(NotEqExpr.get());
+ NotEqExpr =
+ ActOnFinishFullExpr(NotEqExpr.get(), /*DiscardedValue*/ false);
if (NotEqExpr.isInvalid()) {
Diag(RangeLoc, diag::note_for_range_invalid_iterator)
<< RangeLoc << 0 << BeginRangeRef.get()->getType();
@@ -2616,7 +2598,7 @@
// co_await during the initial parse.
IncrExpr = ActOnCoawaitExpr(S, CoawaitLoc, IncrExpr.get());
if (!IncrExpr.isInvalid())
- IncrExpr = ActOnFinishFullExpr(IncrExpr.get());
+ IncrExpr = ActOnFinishFullExpr(IncrExpr.get(), /*DiscardedValue*/ false);
if (IncrExpr.isInvalid()) {
Diag(RangeLoc, diag::note_for_range_invalid_iterator)
<< RangeLoc << 2 << BeginRangeRef.get()->getType() ;
@@ -2871,7 +2853,7 @@
return StmtError();
}
- ExprResult ExprRes = ActOnFinishFullExpr(E);
+ ExprResult ExprRes = ActOnFinishFullExpr(E, /*DiscardedValue*/ false);
if (ExprRes.isInvalid())
return StmtError();
E = ExprRes.get();
@@ -3221,7 +3203,8 @@
ExpressionEvaluationContext::DiscardedStatement &&
(HasDeducedReturnType || CurCap->HasImplicitReturnType)) {
if (RetValExp) {
- ExprResult ER = ActOnFinishFullExpr(RetValExp, ReturnLoc);
+ ExprResult ER =
+ ActOnFinishFullExpr(RetValExp, ReturnLoc, /*DiscardedValue*/ false);
if (ER.isInvalid())
return StmtError();
RetValExp = ER.get();
@@ -3348,7 +3331,8 @@
}
if (RetValExp) {
- ExprResult ER = ActOnFinishFullExpr(RetValExp, ReturnLoc);
+ ExprResult ER =
+ ActOnFinishFullExpr(RetValExp, ReturnLoc, /*DiscardedValue*/ false);
if (ER.isInvalid())
return StmtError();
RetValExp = ER.get();
@@ -3578,7 +3562,8 @@
ExpressionEvaluationContext::DiscardedStatement &&
FnRetType->getContainedAutoType()) {
if (RetValExp) {
- ExprResult ER = ActOnFinishFullExpr(RetValExp, ReturnLoc);
+ ExprResult ER =
+ ActOnFinishFullExpr(RetValExp, ReturnLoc, /*DiscardedValue*/ false);
if (ER.isInvalid())
return StmtError();
RetValExp = ER.get();
@@ -3672,7 +3657,8 @@
}
if (RetValExp) {
- ExprResult ER = ActOnFinishFullExpr(RetValExp, ReturnLoc);
+ ExprResult ER =
+ ActOnFinishFullExpr(RetValExp, ReturnLoc, /*DiscardedValue*/ false);
if (ER.isInvalid())
return StmtError();
RetValExp = ER.get();
@@ -3751,7 +3737,8 @@
}
if (RetValExp) {
- ExprResult ER = ActOnFinishFullExpr(RetValExp, ReturnLoc);
+ ExprResult ER =
+ ActOnFinishFullExpr(RetValExp, ReturnLoc, /*DiscardedValue*/ false);
if (ER.isInvalid())
return StmtError();
RetValExp = ER.get();
@@ -3804,7 +3791,7 @@
if (Result.isInvalid())
return StmtError();
- Result = ActOnFinishFullExpr(Result.get());
+ Result = ActOnFinishFullExpr(Result.get(), /*DiscardedValue*/ false);
if (Result.isInvalid())
return StmtError();
Throw = Result.get();
@@ -3876,7 +3863,7 @@
}
// The operand to @synchronized is a full-expression.
- return ActOnFinishFullExpr(operand);
+ return ActOnFinishFullExpr(operand, /*DiscardedValue*/ false);
}
StmtResult