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.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@350404 91177308-0d34-0410-b5e6-96231b3b80d8

3 files changed, 31 insertions, 11 deletions

diff --git a/lib/Parse/ParseObjc.cpp b/lib/Parse/ParseObjc.cpp
index c8d7bda3d6..bd55f71793 100644
--- a/lib/Parse/ParseObjc.cpp
+++ b/lib/Parse/ParseObjc.cpp
@@ -2741,7 +2741,7 @@ StmtResult Parser::ParseObjCAtStatement(SourceLocation AtLoc) {
 
   // Otherwise, eat the semicolon.
   ExpectAndConsumeSemi(diag::err_expected_semi_after_expr);
-  return Actions.ActOnExprStmt(Res);
+  return Actions.ActOnExprStmt(Res, isExprValueDiscarded());
 }
 
 ExprResult Parser::ParseObjCAtExpression(SourceLocation AtLoc) {
diff --git a/lib/Parse/ParseOpenMP.cpp b/lib/Parse/ParseOpenMP.cpp
index 17c3fa3cf2..dd2a8aae9f 100644
--- a/lib/Parse/ParseOpenMP.cpp
+++ b/lib/Parse/ParseOpenMP.cpp
@@ -314,7 +314,7 @@ Parser::ParseOpenMPDeclareReductionDirective(AccessSpecifier AS) {
     Actions.ActOnOpenMPDeclareReductionCombinerStart(getCurScope(), D);
     ExprResult CombinerResult =
         Actions.ActOnFinishFullExpr(ParseAssignmentExpression().get(),
-                                    D->getLocation(), /*DiscardedValue=*/true);
+                                    D->getLocation(), /*DiscardedValue*/ false);
     Actions.ActOnOpenMPDeclareReductionCombinerEnd(D, CombinerResult.get());
 
     if (CombinerResult.isInvalid() && Tok.isNot(tok::r_paren) &&
@@ -356,7 +356,7 @@ Parser::ParseOpenMPDeclareReductionDirective(AccessSpecifier AS) {
           if (Actions.getLangOpts().CPlusPlus) {
             InitializerResult = Actions.ActOnFinishFullExpr(
                 ParseAssignmentExpression().get(), D->getLocation(),
-                /*DiscardedValue=*/true);
+                /*DiscardedValue*/ false);
           } else {
             ConsumeToken();
             ParseOpenMPReductionInitializerForDecl(OmpPrivParm);
@@ -364,7 +364,7 @@ Parser::ParseOpenMPDeclareReductionDirective(AccessSpecifier AS) {
         } else {
           InitializerResult = Actions.ActOnFinishFullExpr(
               ParseAssignmentExpression().get(), D->getLocation(),
-              /*DiscardedValue=*/true);
+              /*DiscardedValue*/ false);
         }
         Actions.ActOnOpenMPDeclareReductionInitializerEnd(
             D, InitializerResult.get(), OmpPrivParm);
@@ -1455,7 +1455,7 @@ ExprResult Parser::ParseOpenMPParensExpr(StringRef ClauseName,
   ExprResult LHS(ParseCastExpression(
       /*isUnaryExpression=*/false, /*isAddressOfOperand=*/false, NotTypeCast));
   ExprResult Val(ParseRHSOfBinaryExpression(LHS, prec::Conditional));
-  Val = Actions.ActOnFinishFullExpr(Val.get(), ELoc);
+  Val = Actions.ActOnFinishFullExpr(Val.get(), ELoc, /*DiscardedValue*/ false);
 
   // Parse ')'.
   RLoc = Tok.getLocation();
@@ -1711,7 +1711,8 @@ OMPClause *Parser::ParseOpenMPSingleExprWithArgClause(OpenMPClauseKind Kind,
     SourceLocation ELoc = Tok.getLocation();
     ExprResult LHS(ParseCastExpression(false, false, NotTypeCast));
     Val = ParseRHSOfBinaryExpression(LHS, prec::Conditional);
-    Val = Actions.ActOnFinishFullExpr(Val.get(), ELoc);
+    Val =
+        Actions.ActOnFinishFullExpr(Val.get(), ELoc, /*DiscardedValue*/ false);
   }
 
   // Parse ')'.
@@ -1996,7 +1997,8 @@ bool Parser::ParseOpenMPVarList(OpenMPDirectiveKind DKind,
     Data.ColonLoc = Tok.getLocation();
     SourceLocation ELoc = ConsumeToken();
     ExprResult Tail = ParseAssignmentExpression();
-    Tail = Actions.ActOnFinishFullExpr(Tail.get(), ELoc);
+    Tail =
+        Actions.ActOnFinishFullExpr(Tail.get(), ELoc, /*DiscardedValue*/ false);
     if (Tail.isUsable())
       Data.TailExpr = Tail.get();
     else
diff --git a/lib/Parse/ParseStmt.cpp b/lib/Parse/ParseStmt.cpp
index 313793c3e8..2974e6a245 100644
--- a/lib/Parse/ParseStmt.cpp
+++ b/lib/Parse/ParseStmt.cpp
@@ -439,7 +439,7 @@ StmtResult Parser::ParseExprStatement() {
 
   // Otherwise, eat the semicolon.
   ExpectAndConsumeSemi(diag::err_expected_semi_after_expr);
-  return Actions.ActOnExprStmt(Expr);
+  return Actions.ActOnExprStmt(Expr, isExprValueDiscarded());
 }
 
 /// ParseSEHTryBlockCommon
@@ -958,6 +958,16 @@ bool Parser::ConsumeNullStmt(StmtVector &Stmts) {
   return true;
 }
 
+bool Parser::isExprValueDiscarded() {
+  if (Actions.isCurCompoundStmtAStmtExpr()) {
+    // Look to see if the next two tokens close the statement expression;
+    // if so, this expression statement is the last statement in a
+    // statment expression.
+    return Tok.isNot(tok::r_brace) || NextToken().isNot(tok::r_paren);
+  }
+  return true;
+}
+
 /// ParseCompoundStatementBody - Parse a sequence of statements and invoke the
 /// ActOnCompoundStmt action.  This expects the '{' to be the current token, and
 /// consume the '}' at the end of the block.  It does not manipulate the scope
@@ -1062,7 +1072,7 @@ StmtResult Parser::ParseCompoundStatementBody(bool isStmtExpr) {
         // Eat the semicolon at the end of stmt and convert the expr into a
         // statement.
         ExpectAndConsumeSemi(diag::err_expected_semi_after_expr);
-        R = Actions.ActOnExprStmt(Res);
+        R = Actions.ActOnExprStmt(Res, isExprValueDiscarded());
       }
     }
 
@@ -1698,8 +1708,16 @@ StmtResult Parser::ParseForStatement(SourceLocation *TrailingElseLoc) {
     if (!Value.isInvalid()) {
       if (ForEach)
         FirstPart = Actions.ActOnForEachLValueExpr(Value.get());
-      else
-        FirstPart = Actions.ActOnExprStmt(Value);
+      else {
+        // We already know this is not an init-statement within a for loop, so
+        // if we are parsing a C++11 range-based for loop, we should treat this
+        // expression statement as being a discarded value expression because
+        // we will err below. This way we do not warn on an unused expression
+        // that was an error in the first place, like with: for (expr : expr);
+        bool IsRangeBasedFor =
+            getLangOpts().CPlusPlus11 && !ForEach && Tok.is(tok::colon);
+        FirstPart = Actions.ActOnExprStmt(Value, !IsRangeBasedFor);
+      }
     }
 
     if (Tok.is(tok::semi)) {