aaron.ballman updated this revision to Diff 179475.
aaron.ballman marked 3 inline comments as done.
aaron.ballman added a comment.
Updated based on review feedback.
The lookahead wasn't too annoying to thread through, but I did run into
surprises in TreeTransform where I can't use the lookahead to determine whether
the expression statement should be warned on or not. The solution I have is
serviceable, but perhaps there's a better approach to be taken.
The changes introduce warnings where they were previously missed, such as in
init-statements. The only unfortunate behavioral side effect comes from
range-based for loops that have an expression as the first statement -- in
addition to the error, we now trigger an unused expression result warning, but
restructuring the code to avoid it would be challenging for such an edge case
(and the warning behavior seems reasonable).
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55955/new/
https://reviews.llvm.org/D55955
Files:
include/clang/Parse/Parser.h
include/clang/Sema/Sema.h
lib/Parse/ParseObjc.cpp
lib/Parse/ParseOpenMP.cpp
lib/Parse/ParseStmt.cpp
lib/Sema/SemaExprCXX.cpp
lib/Sema/SemaOpenMP.cpp
lib/Sema/SemaStmt.cpp
lib/Sema/TreeTransform.h
test/CXX/stmt.stmt/stmt.select/p3.cpp
test/CodeCompletion/pragma-macro-token-caching.c
test/Parser/cxx1z-init-statement.cpp
test/Parser/switch-recovery.cpp
test/SemaCXX/cxx1z-init-statement.cpp
test/SemaCXX/for-range-examples.cpp
test/SemaCXX/warn-unused-result.cpp
test/SemaObjCXX/foreach.mm
Index: test/SemaObjCXX/foreach.mm
===================================================================
--- test/SemaObjCXX/foreach.mm
+++ test/SemaObjCXX/foreach.mm
@@ -6,8 +6,8 @@
void f(NSArray *a) {
id keys;
for (int i : a); // expected-error{{selector element type 'int' is not a valid object}}
- for ((id)2 : a); // expected-error {{for range declaration must declare a variable}}
- for (2 : a); // expected-error {{for range declaration must declare a variable}}
+ for ((id)2 : a); // expected-error {{for range declaration must declare a variable}} expected-warning {{expression result unused}}
+ for (2 : a); // expected-error {{for range declaration must declare a variable}} expected-warning {{expression result unused}}
for (id thisKey : keys);
@@ -71,7 +71,7 @@
@end
void test2(NSObject<NSFastEnumeration> *collection) {
Test2 *obj;
- for (obj.prop : collection) { // expected-error {{for range declaration must declare a variable}}
+ for (obj.prop : collection) { // expected-error {{for range declaration must declare a variable}} expected-warning {{property access result unused}}
}
}
Index: test/SemaCXX/warn-unused-result.cpp
===================================================================
--- test/SemaCXX/warn-unused-result.cpp
+++ test/SemaCXX/warn-unused-result.cpp
@@ -33,6 +33,36 @@
const S &s4 = g1();
}
+void testSubstmts(int i) {
+ switch (i) {
+ case 0:
+ f(); // expected-warning {{ignoring return value}}
+ default:
+ f(); // expected-warning {{ignoring return value}}
+ }
+
+ if (i)
+ f(); // expected-warning {{ignoring return value}}
+ else
+ f(); // expected-warning {{ignoring return value}}
+
+ while (i)
+ f(); // expected-warning {{ignoring return value}}
+
+ do
+ f(); // expected-warning {{ignoring return value}}
+ while (i);
+
+ for (f(); // expected-warning {{ignoring return value}}
+ ;
+ f() // expected-warning {{ignoring return value}}
+ )
+ f(); // expected-warning {{ignoring return value}}
+
+ f(), // expected-warning {{ignoring return value}}
+ (void)f();
+}
+
struct X {
int foo() __attribute__((warn_unused_result));
};
@@ -206,3 +236,13 @@
(void)++p;
}
} // namespace
+
+namespace PR39837 {
+[[clang::warn_unused_result]] int f(int);
+
+void g() {
+ int a[2];
+ for (int b : a)
+ f(b); // expected-warning {{ignoring return value}}
+}
+} // namespace PR39837
Index: test/SemaCXX/for-range-examples.cpp
===================================================================
--- test/SemaCXX/for-range-examples.cpp
+++ test/SemaCXX/for-range-examples.cpp
@@ -176,9 +176,9 @@
// Make sure these don't crash. Better diagnostics would be nice.
for (: {1, 2, 3}) {} // expected-error {{expected expression}} expected-error {{expected ';'}}
- for (1 : {1, 2, 3}) {} // expected-error {{must declare a variable}}
+ for (1 : {1, 2, 3}) {} // expected-error {{must declare a variable}} expected-warning {{expression result unused}}
for (+x : {1, 2, 3}) {} // expected-error {{undeclared identifier}} expected-error {{expected ';'}}
- for (+y : {1, 2, 3}) {} // expected-error {{must declare a variable}}
+ for (+y : {1, 2, 3}) {} // expected-error {{must declare a variable}} expected-warning {{expression result unused}}
}
}
@@ -244,7 +244,7 @@
{
int b = 1, a[b];
a[0] = 0;
- [&] { for (int c : a) 0; } ();
+ [&] { for (int c : a) 0; } (); // expected-warning {{expression result unused}}
}
Index: test/SemaCXX/cxx1z-init-statement.cpp
===================================================================
--- test/SemaCXX/cxx1z-init-statement.cpp
+++ test/SemaCXX/cxx1z-init-statement.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -std=c++1z -verify %s
-// RUN: %clang_cc1 -std=c++17 -verify %s
+// RUN: %clang_cc1 -std=c++1z -Wno-unused-value -verify %s
+// RUN: %clang_cc1 -std=c++17 -Wno-unused-value -verify %s
void testIf() {
int x = 0;
@@ -12,7 +12,7 @@
int x = 0; // expected-error {{redefinition of 'x'}}
if (x; int a = 0) ++a;
- if (x, +x; int a = 0) // expected-note 2 {{previous definition is here}} expected-warning {{unused}}
+ if (x, +x; int a = 0) // expected-note 2 {{previous definition is here}}
int a = 0; // expected-error {{redefinition of 'a'}}
else
int a = 0; // expected-error {{redefinition of 'a'}}
@@ -48,7 +48,7 @@
++a;
}
- switch (x, +x; int a = 0) { // expected-note {{previous definition is here}} expected-warning {{unused}}
+ switch (x, +x; int a = 0) { // expected-note {{previous definition is here}}
case 0:
int a = 0; // expected-error {{redefinition of 'a'}} // expected-note {{previous definition is here}}
case 1:
Index: test/Parser/switch-recovery.cpp
===================================================================
--- test/Parser/switch-recovery.cpp
+++ test/Parser/switch-recovery.cpp
@@ -105,7 +105,7 @@
expected-error {{expected expression}}
8:: x; // expected-error {{expected ';' after expression}} \
expected-error {{no member named 'x' in the global namespace; did you mean simply 'x'?}} \
- expected-warning 2 {{expression result unused}}
+ expected-warning {{expression result unused}}
9:: :y; // expected-error {{expected ';' after expression}} \
expected-error {{expected unqualified-id}} \
expected-warning {{expression result unused}}
Index: test/Parser/cxx1z-init-statement.cpp
===================================================================
--- test/Parser/cxx1z-init-statement.cpp
+++ test/Parser/cxx1z-init-statement.cpp
@@ -13,9 +13,9 @@
if (T(n) = 0; n) {}
// init-statement expressions
- if (T{f()}; f()) {}
- if (T{f()}, g, h; f()) {} // expected-warning 2{{unused}}
- if (T(f()), g, h + 1; f()) {} // expected-warning 2{{unused}}
+ if (T{f()}; f()) {} // expected-warning {{expression result unused}}
+ if (T{f()}, g, h; f()) {} // expected-warning 2{{unused}} expected-warning {{expression result unused}}
+ if (T(f()), g, h + 1; f()) {} // expected-warning 2{{unused}} expected-warning {{expression result unused}}
// condition declarations
if (T(n){g}) {}
@@ -35,7 +35,7 @@
// Likewise for 'switch'
switch (int n; n) {}
- switch (g; int g = 5) {}
+ switch (g; int g = 5) {} // expected-warning {{expression result unused}}
if (int a, b; int c = a) { // expected-note 6{{previous}}
int a; // expected-error {{redefinition}}
Index: test/CodeCompletion/pragma-macro-token-caching.c
===================================================================
--- test/CodeCompletion/pragma-macro-token-caching.c
+++ test/CodeCompletion/pragma-macro-token-caching.c
@@ -12,7 +12,7 @@
void completeParamPragmaError(int param) {
Outer(__extension__({ _Pragma(2) })); // expected-error {{_Pragma takes a parenthesized string literal}}
- param;
+ param; // expected-warning {{expression result unused}}
}
// RUN: %clang_cc1 -fsyntax-only -verify -code-completion-at=%s:16:1 %s | FileCheck %s
Index: test/CXX/stmt.stmt/stmt.select/p3.cpp
===================================================================
--- test/CXX/stmt.stmt/stmt.select/p3.cpp
+++ test/CXX/stmt.stmt/stmt.select/p3.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
-// RUN: %clang_cc1 -fsyntax-only -std=c++1z -Wc++14-compat -verify %s -DCPP17
+// RUN: %clang_cc1 -fsyntax-only -Wno-unused-value -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-unused-value -std=c++1z -Wc++14-compat -verify %s -DCPP17
int f();
@@ -71,7 +71,6 @@
// last loop above. It would be nice to remove this.
void whileInitStatement2() {
while (; false) {} // expected-error {{expected expression}}
- // expected-warning@-1 {{expression result unused}}
- // expected-error@-2 {{expected ';' after expression}}
- // expected-error@-3 {{expected expression}}
+ // expected-error@-1 {{expected ';' after expression}}
+ // expected-error@-2 {{expected expression}}
}
Index: lib/Sema/TreeTransform.h
===================================================================
--- lib/Sema/TreeTransform.h
+++ lib/Sema/TreeTransform.h
@@ -111,6 +111,8 @@
}
};
+ unsigned SuppressWarnOnUnusedExpr = 0;
+
protected:
Sema &SemaRef;
@@ -3292,7 +3294,7 @@
if (E.isInvalid())
return StmtError();
- return getSema().ActOnExprStmt(E);
+ return getSema().ActOnExprStmt(E, SuppressWarnOnUnusedExpr == 0);
}
}
@@ -6518,7 +6520,13 @@
bool SubStmtChanged = false;
SmallVector<Stmt*, 8> Statements;
for (auto *B : S->body()) {
+ bool TransformingLastStmt = B == S->body_back();
+ if (IsStmtExpr && TransformingLastStmt)
+ ++SuppressWarnOnUnusedExpr;
StmtResult Result = getDerived().TransformStmt(B);
+ if (IsStmtExpr && TransformingLastStmt)
+ --SuppressWarnOnUnusedExpr;
+
if (Result.isInvalid()) {
// Immediately fail if this was a DeclStmt, since it's very
// likely that this will cause problems for future statements.
@@ -7354,12 +7362,16 @@
TreeTransform<Derived>::TransformObjCForCollectionStmt(
ObjCForCollectionStmt *S) {
// Transform the element statement.
+ ++SuppressWarnOnUnusedExpr;
StmtResult Element = getDerived().TransformStmt(S->getElement());
+ --SuppressWarnOnUnusedExpr;
if (Element.isInvalid())
return StmtError();
// Transform the collection expression.
+ ++SuppressWarnOnUnusedExpr;
ExprResult Collection = getDerived().TransformExpr(S->getCollection());
+ --SuppressWarnOnUnusedExpr;
if (Collection.isInvalid())
return StmtError();
Index: lib/Sema/SemaStmt.cpp
===================================================================
--- lib/Sema/SemaStmt.cpp
+++ lib/Sema/SemaStmt.cpp
@@ -42,12 +42,13 @@
using namespace clang;
using namespace sema;
-StmtResult Sema::ActOnExprStmt(ExprResult FE) {
+StmtResult Sema::ActOnExprStmt(ExprResult FE,
+ bool WarnOnDiscardedValue) {
if (FE.isInvalid())
return StmtError();
FE = ActOnFinishFullExpr(FE.get(), FE.get()->getExprLoc(),
- /*DiscardedValue*/ true);
+ WarnOnDiscardedValue);
if (FE.isInvalid())
return StmtError();
@@ -371,14 +372,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
@@ -470,15 +463,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;
@@ -572,9 +562,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);
}
@@ -1302,8 +1289,6 @@
!Diags.isIgnored(diag::warn_comma_operator, CondVal.second->getExprLoc()))
CommaVisitor(*this).Visit(CondVal.second);
- DiagnoseUnusedExprResult(Body);
-
if (isa<NullStmt>(Body))
getCurCompoundScope().setHasEmptyLoopBodies();
@@ -1333,8 +1318,6 @@
!Diags.isIgnored(diag::warn_comma_operator, Cond->getExprLoc()))
CommaVisitor(*this).Visit(Cond);
- DiagnoseUnusedExprResult(Body);
-
return new (Context) DoStmt(Body, Cond, DoLoc, WhileLoc, CondRParen);
}
@@ -1779,11 +1762,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();
Index: lib/Sema/SemaOpenMP.cpp
===================================================================
--- lib/Sema/SemaOpenMP.cpp
+++ lib/Sema/SemaOpenMP.cpp
@@ -10243,8 +10243,7 @@
PseudoDstExpr, PseudoSrcExpr);
if (AssignmentOp.isInvalid())
continue;
- AssignmentOp = ActOnFinishFullExpr(AssignmentOp.get(), ELoc,
- /*DiscardedValue=*/true);
+ AssignmentOp = ActOnFinishFullExpr(AssignmentOp.get(), ELoc);
if (AssignmentOp.isInvalid())
continue;
@@ -11675,8 +11674,7 @@
InitExpr, IV, Step, /* Subtract */ false);
else
Update = *CurPrivate;
- Update = SemaRef.ActOnFinishFullExpr(Update.get(), DE->getBeginLoc(),
- /*DiscardedValue=*/true);
+ Update = SemaRef.ActOnFinishFullExpr(Update.get(), DE->getBeginLoc());
// Build final: Var = InitExpr + NumIterations * Step
ExprResult Final;
@@ -11686,8 +11684,7 @@
InitExpr, NumIterations, Step, /*Subtract=*/false);
else
Final = *CurPrivate;
- Final = SemaRef.ActOnFinishFullExpr(Final.get(), DE->getBeginLoc(),
- /*DiscardedValue=*/true);
+ Final = SemaRef.ActOnFinishFullExpr(Final.get(), DE->getBeginLoc());
if (!Update.isUsable() || !Final.isUsable()) {
Updates.push_back(nullptr);
@@ -11854,8 +11851,7 @@
PseudoSrcExpr);
if (AssignmentOp.isInvalid())
continue;
- AssignmentOp = ActOnFinishFullExpr(AssignmentOp.get(), DE->getExprLoc(),
- /*DiscardedValue=*/true);
+ AssignmentOp = ActOnFinishFullExpr(AssignmentOp.get(), DE->getExprLoc());
if (AssignmentOp.isInvalid())
continue;
@@ -11963,8 +11959,7 @@
DSAStack->getCurScope(), ELoc, BO_Assign, PseudoDstExpr, PseudoSrcExpr);
if (AssignmentOp.isInvalid())
continue;
- AssignmentOp = ActOnFinishFullExpr(AssignmentOp.get(), ELoc,
- /*DiscardedValue=*/true);
+ AssignmentOp = ActOnFinishFullExpr(AssignmentOp.get(), ELoc);
if (AssignmentOp.isInvalid())
continue;
Index: lib/Sema/SemaExprCXX.cpp
===================================================================
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -7772,7 +7772,7 @@
}
ExprResult Sema::ActOnFinishFullExpr(Expr *FE, SourceLocation CC,
- bool DiscardedValue,
+ bool WarnOnDiscardedValue,
bool IsConstexpr) {
ExprResult FullExpr = FE;
@@ -7782,7 +7782,7 @@
if (DiagnoseUnexpandedParameterPack(FullExpr.get()))
return ExprError();
- if (DiscardedValue) {
+ if (WarnOnDiscardedValue) {
// Top-level expressions default to 'id' when we're in a debugger.
if (getLangOpts().DebuggerCastResultToId &&
FullExpr.get()->getType() == Context.UnknownAnyTy) {
@@ -7798,6 +7798,8 @@
FullExpr = IgnoredValueConversions(FullExpr.get());
if (FullExpr.isInvalid())
return ExprError();
+
+ DiagnoseUnusedExprResult(FullExpr.get());
}
FullExpr = CorrectDelayedTyposInExpr(FullExpr.get());
Index: lib/Parse/ParseStmt.cpp
===================================================================
--- lib/Parse/ParseStmt.cpp
+++ lib/Parse/ParseStmt.cpp
@@ -20,6 +20,7 @@
#include "clang/Parse/RAIIObjectsForParser.h"
#include "clang/Sema/DeclSpec.h"
#include "clang/Sema/Scope.h"
+#include "clang/Sema/ScopeInfo.h"
#include "clang/Sema/TypoCorrection.h"
using namespace clang;
@@ -439,7 +440,7 @@
// Otherwise, eat the semicolon.
ExpectAndConsumeSemi(diag::err_expected_semi_after_expr);
- return Actions.ActOnExprStmt(Expr);
+ return Actions.ActOnExprStmt(Expr, warnOnDiscardedValueExpr());
}
/// ParseSEHTryBlockCommon
@@ -958,6 +959,16 @@
return true;
}
+bool Parser::warnOnDiscardedValueExpr() {
+ if (Actions.getCurCompoundScope().IsStmtExpr) {
+ // 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 +1073,7 @@
// 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, warnOnDiscardedValueExpr());
}
}
Index: lib/Parse/ParseOpenMP.cpp
===================================================================
--- lib/Parse/ParseOpenMP.cpp
+++ lib/Parse/ParseOpenMP.cpp
@@ -312,9 +312,8 @@
Scope::OpenMPDirectiveScope);
// Parse <combiner> expression.
Actions.ActOnOpenMPDeclareReductionCombinerStart(getCurScope(), D);
- ExprResult CombinerResult =
- Actions.ActOnFinishFullExpr(ParseAssignmentExpression().get(),
- D->getLocation(), /*DiscardedValue=*/true);
+ ExprResult CombinerResult = Actions.ActOnFinishFullExpr(
+ ParseAssignmentExpression().get(), D->getLocation());
Actions.ActOnOpenMPDeclareReductionCombinerEnd(D, CombinerResult.get());
if (CombinerResult.isInvalid() && Tok.isNot(tok::r_paren) &&
@@ -355,16 +354,14 @@
Tok.getIdentifierInfo()->isStr("omp_priv")) {
if (Actions.getLangOpts().CPlusPlus) {
InitializerResult = Actions.ActOnFinishFullExpr(
- ParseAssignmentExpression().get(), D->getLocation(),
- /*DiscardedValue=*/true);
+ ParseAssignmentExpression().get(), D->getLocation());
} else {
ConsumeToken();
ParseOpenMPReductionInitializerForDecl(OmpPrivParm);
}
} else {
InitializerResult = Actions.ActOnFinishFullExpr(
- ParseAssignmentExpression().get(), D->getLocation(),
- /*DiscardedValue=*/true);
+ ParseAssignmentExpression().get(), D->getLocation());
}
Actions.ActOnOpenMPDeclareReductionInitializerEnd(
D, InitializerResult.get(), OmpPrivParm);
Index: lib/Parse/ParseObjc.cpp
===================================================================
--- lib/Parse/ParseObjc.cpp
+++ lib/Parse/ParseObjc.cpp
@@ -2741,7 +2741,7 @@
// Otherwise, eat the semicolon.
ExpectAndConsumeSemi(diag::err_expected_semi_after_expr);
- return Actions.ActOnExprStmt(Res);
+ return Actions.ActOnExprStmt(Res, warnOnDiscardedValueExpr());
}
ExprResult Parser::ParseObjCAtExpression(SourceLocation AtLoc) {
Index: include/clang/Sema/Sema.h
===================================================================
--- include/clang/Sema/Sema.h
+++ include/clang/Sema/Sema.h
@@ -3692,7 +3692,7 @@
return FullExprArg(FE.get());
}
- StmtResult ActOnExprStmt(ExprResult Arg);
+ StmtResult ActOnExprStmt(ExprResult Arg, bool WarnOnDiscardedValue = true);
StmtResult ActOnExprStmtError();
StmtResult ActOnNullStmt(SourceLocation SemiLoc,
@@ -5334,7 +5334,7 @@
: SourceLocation());
}
ExprResult ActOnFinishFullExpr(Expr *Expr, SourceLocation CC,
- bool DiscardedValue = false,
+ bool WarnOnDiscardedValue = false,
bool IsConstexpr = false);
StmtResult ActOnFinishFullStmt(Stmt *Stmt);
Index: include/clang/Parse/Parser.h
===================================================================
--- include/clang/Parse/Parser.h
+++ include/clang/Parse/Parser.h
@@ -359,6 +359,8 @@
/// just a regular sub-expression.
SourceLocation ExprStatementTokLoc;
+ bool warnOnDiscardedValueExpr();
+
public:
Parser(Preprocessor &PP, Sema &Actions, bool SkipFunctionBodies);
~Parser() override;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
