Skip to content

Commit 6e3c4d5

Browse files
zmodemzmodem
committed
Revert r350404
This caused https://bugs.llvm.org/show_bug.cgi?id=40642: "After 350404, clang drops volatile load" > 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: 353935
1 parent be8c9e3 commit 6e3c4d5

21 files changed

+122
-196
lines changed

‎clang/include/clang/Parse/Parser.h

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -360,11 +360,6 @@ class Parser : public CodeCompletionHandler {
360360
/// just a regular sub-expression.
361361
SourceLocation ExprStatementTokLoc;
362362

363-
/// Tests whether an expression value is discarded based on token lookahead.
364-
/// It will return true if the lexer is currently processing the })
365-
/// terminating a GNU statement expression and false otherwise.
366-
bool isExprValueDiscarded();
367-
368363
public:
369364
Parser(Preprocessor &PP, Sema &Actions, bool SkipFunctionBodies);
370365
~Parser() override;

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

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1369,7 +1369,6 @@ class Sema {
13691369
void PopCompoundScope();
13701370

13711371
sema::CompoundScopeInfo &getCurCompoundScope() const;
1372-
bool isCurCompoundStmtAStmtExpr() const;
13731372

13741373
bool hasAnyUnrecoverableErrorsInThisFunction() const;
13751374

@@ -3690,17 +3689,16 @@ class Sema {
36903689
return MakeFullExpr(Arg, Arg ? Arg->getExprLoc() : SourceLocation());
36913690
}
36923691
FullExprArg MakeFullExpr(Expr *Arg, SourceLocation CC) {
3693-
return FullExprArg(
3694-
ActOnFinishFullExpr(Arg, CC, /*DiscardedValue*/ false).get());
3692+
return FullExprArg(ActOnFinishFullExpr(Arg, CC).get());
36953693
}
36963694
FullExprArg MakeFullDiscardedValueExpr(Expr *Arg) {
36973695
ExprResult FE =
3698-
ActOnFinishFullExpr(Arg, Arg ? Arg->getExprLoc() : SourceLocation(),
3699-
/*DiscardedValue*/ true);
3696+
ActOnFinishFullExpr(Arg, Arg ? Arg->getExprLoc() : SourceLocation(),
3697+
/*DiscardedValue*/ true);
37003698
return FullExprArg(FE.get());
37013699
}
37023700

3703-
StmtResult ActOnExprStmt(ExprResult Arg, bool DiscardedValue = true);
3701+
StmtResult ActOnExprStmt(ExprResult Arg);
37043702
StmtResult ActOnExprStmtError();
37053703

37063704
StmtResult ActOnNullStmt(SourceLocation SemiLoc,
@@ -5346,12 +5344,13 @@ class Sema {
53465344
CreateMaterializeTemporaryExpr(QualType T, Expr *Temporary,
53475345
bool BoundToLvalueReference);
53485346

5349-
ExprResult ActOnFinishFullExpr(Expr *Expr, bool DiscardedValue) {
5350-
return ActOnFinishFullExpr(
5351-
Expr, Expr ? Expr->getExprLoc() : SourceLocation(), DiscardedValue);
5347+
ExprResult ActOnFinishFullExpr(Expr *Expr) {
5348+
return ActOnFinishFullExpr(Expr, Expr ? Expr->getExprLoc()
5349+
: SourceLocation());
53525350
}
53535351
ExprResult ActOnFinishFullExpr(Expr *Expr, SourceLocation CC,
5354-
bool DiscardedValue, bool IsConstexpr = false);
5352+
bool DiscardedValue = false,
5353+
bool IsConstexpr = false);
53555354
StmtResult ActOnFinishFullStmt(Stmt *Stmt);
53565355

53575356
// Marks SS invalid if it represents an incomplete type.

‎clang/lib/Parse/ParseObjc.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2741,7 +2741,7 @@ StmtResult Parser::ParseObjCAtStatement(SourceLocation AtLoc) {
27412741

27422742
// Otherwise, eat the semicolon.
27432743
ExpectAndConsumeSemi(diag::err_expected_semi_after_expr);
2744-
return Actions.ActOnExprStmt(Res, isExprValueDiscarded());
2744+
return Actions.ActOnExprStmt(Res);
27452745
}
27462746

27472747
ExprResult Parser::ParseObjCAtExpression(SourceLocation AtLoc) {

‎clang/lib/Parse/ParseOpenMP.cpp

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -314,7 +314,7 @@ Parser::ParseOpenMPDeclareReductionDirective(AccessSpecifier AS) {
314314
Actions.ActOnOpenMPDeclareReductionCombinerStart(getCurScope(), D);
315315
ExprResult CombinerResult =
316316
Actions.ActOnFinishFullExpr(ParseAssignmentExpression().get(),
317-
D->getLocation(), /*DiscardedValue*/ false);
317+
D->getLocation(), /*DiscardedValue=*/true);
318318
Actions.ActOnOpenMPDeclareReductionCombinerEnd(D, CombinerResult.get());
319319

320320
if (CombinerResult.isInvalid() && Tok.isNot(tok::r_paren) &&
@@ -356,15 +356,15 @@ Parser::ParseOpenMPDeclareReductionDirective(AccessSpecifier AS) {
356356
if (Actions.getLangOpts().CPlusPlus) {
357357
InitializerResult = Actions.ActOnFinishFullExpr(
358358
ParseAssignmentExpression().get(), D->getLocation(),
359-
/*DiscardedValue*/ false);
359+
/*DiscardedValue=*/true);
360360
} else {
361361
ConsumeToken();
362362
ParseOpenMPReductionInitializerForDecl(OmpPrivParm);
363363
}
364364
} else {
365365
InitializerResult = Actions.ActOnFinishFullExpr(
366366
ParseAssignmentExpression().get(), D->getLocation(),
367-
/*DiscardedValue*/ false);
367+
/*DiscardedValue=*/true);
368368
}
369369
Actions.ActOnOpenMPDeclareReductionInitializerEnd(
370370
D, InitializerResult.get(), OmpPrivParm);
@@ -1455,7 +1455,7 @@ ExprResult Parser::ParseOpenMPParensExpr(StringRef ClauseName,
14551455
ExprResult LHS(ParseCastExpression(
14561456
/*isUnaryExpression=*/false, /*isAddressOfOperand=*/false, NotTypeCast));
14571457
ExprResult Val(ParseRHSOfBinaryExpression(LHS, prec::Conditional));
1458-
Val = Actions.ActOnFinishFullExpr(Val.get(), ELoc, /*DiscardedValue*/ false);
1458+
Val = Actions.ActOnFinishFullExpr(Val.get(), ELoc);
14591459

14601460
// Parse ')'.
14611461
RLoc = Tok.getLocation();
@@ -1711,8 +1711,7 @@ OMPClause *Parser::ParseOpenMPSingleExprWithArgClause(OpenMPClauseKind Kind,
17111711
SourceLocation ELoc = Tok.getLocation();
17121712
ExprResult LHS(ParseCastExpression(false, false, NotTypeCast));
17131713
Val = ParseRHSOfBinaryExpression(LHS, prec::Conditional);
1714-
Val =
1715-
Actions.ActOnFinishFullExpr(Val.get(), ELoc, /*DiscardedValue*/ false);
1714+
Val = Actions.ActOnFinishFullExpr(Val.get(), ELoc);
17161715
}
17171716

17181717
// Parse ')'.
@@ -1997,8 +1996,7 @@ bool Parser::ParseOpenMPVarList(OpenMPDirectiveKind DKind,
19971996
Data.ColonLoc = Tok.getLocation();
19981997
SourceLocation ELoc = ConsumeToken();
19991998
ExprResult Tail = ParseAssignmentExpression();
2000-
Tail =
2001-
Actions.ActOnFinishFullExpr(Tail.get(), ELoc, /*DiscardedValue*/ false);
1999+
Tail = Actions.ActOnFinishFullExpr(Tail.get(), ELoc);
20022000
if (Tail.isUsable())
20032001
Data.TailExpr = Tail.get();
20042002
else

‎clang/lib/Parse/ParseStmt.cpp

Lines changed: 4 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -439,7 +439,7 @@ StmtResult Parser::ParseExprStatement() {
439439

440440
// Otherwise, eat the semicolon.
441441
ExpectAndConsumeSemi(diag::err_expected_semi_after_expr);
442-
return Actions.ActOnExprStmt(Expr, isExprValueDiscarded());
442+
return Actions.ActOnExprStmt(Expr);
443443
}
444444

445445
/// ParseSEHTryBlockCommon
@@ -958,16 +958,6 @@ bool Parser::ConsumeNullStmt(StmtVector &Stmts) {
958958
return true;
959959
}
960960

961-
bool Parser::isExprValueDiscarded() {
962-
if (Actions.isCurCompoundStmtAStmtExpr()) {
963-
// Look to see if the next two tokens close the statement expression;
964-
// if so, this expression statement is the last statement in a
965-
// statment expression.
966-
return Tok.isNot(tok::r_brace) || NextToken().isNot(tok::r_paren);
967-
}
968-
return true;
969-
}
970-
971961
/// ParseCompoundStatementBody - Parse a sequence of statements and invoke the
972962
/// ActOnCompoundStmt action. This expects the '{' to be the current token, and
973963
/// consume the '}' at the end of the block. It does not manipulate the scope
@@ -1072,7 +1062,7 @@ StmtResult Parser::ParseCompoundStatementBody(bool isStmtExpr) {
10721062
// Eat the semicolon at the end of stmt and convert the expr into a
10731063
// statement.
10741064
ExpectAndConsumeSemi(diag::err_expected_semi_after_expr);
1075-
R = Actions.ActOnExprStmt(Res, isExprValueDiscarded());
1065+
R = Actions.ActOnExprStmt(Res);
10761066
}
10771067
}
10781068

@@ -1708,16 +1698,8 @@ StmtResult Parser::ParseForStatement(SourceLocation *TrailingElseLoc) {
17081698
if (!Value.isInvalid()) {
17091699
if (ForEach)
17101700
FirstPart = Actions.ActOnForEachLValueExpr(Value.get());
1711-
else {
1712-
// We already know this is not an init-statement within a for loop, so
1713-
// if we are parsing a C++11 range-based for loop, we should treat this
1714-
// expression statement as being a discarded value expression because
1715-
// we will err below. This way we do not warn on an unused expression
1716-
// that was an error in the first place, like with: for (expr : expr);
1717-
bool IsRangeBasedFor =
1718-
getLangOpts().CPlusPlus11 && !ForEach && Tok.is(tok::colon);
1719-
FirstPart = Actions.ActOnExprStmt(Value, !IsRangeBasedFor);
1720-
}
1701+
else
1702+
FirstPart = Actions.ActOnExprStmt(Value);
17211703
}
17221704

17231705
if (Tok.is(tok::semi)) {

‎clang/lib/Sema/SemaCoroutine.cpp

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -646,7 +646,7 @@ bool Sema::ActOnCoroutineBodyStart(Scope *SC, SourceLocation KWLoc,
646646
return StmtError();
647647
Suspend = BuildResolvedCoawaitExpr(Loc, Suspend.get(),
648648
/*IsImplicit*/ true);
649-
Suspend = ActOnFinishFullExpr(Suspend.get(), /*DiscardedValue*/ false);
649+
Suspend = ActOnFinishFullExpr(Suspend.get());
650650
if (Suspend.isInvalid()) {
651651
Diag(Loc, diag::note_coroutine_promise_suspend_implicitly_required)
652652
<< ((Name == "initial_suspend") ? 0 : 1);
@@ -867,7 +867,7 @@ StmtResult Sema::BuildCoreturnStmt(SourceLocation Loc, Expr *E,
867867
if (PC.isInvalid())
868868
return StmtError();
869869

870-
Expr *PCE = ActOnFinishFullExpr(PC.get(), /*DiscardedValue*/ false).get();
870+
Expr *PCE = ActOnFinishFullExpr(PC.get()).get();
871871

872872
Stmt *Res = new (Context) CoreturnStmt(Loc, E, PCE, IsImplicit);
873873
return Res;
@@ -1236,7 +1236,7 @@ bool CoroutineStmtBuilder::makeNewAndDeleteExpr() {
12361236

12371237
ExprResult NewExpr =
12381238
S.ActOnCallExpr(S.getCurScope(), NewRef.get(), Loc, NewArgs, Loc);
1239-
NewExpr = S.ActOnFinishFullExpr(NewExpr.get(), /*DiscardedValue*/ false);
1239+
NewExpr = S.ActOnFinishFullExpr(NewExpr.get());
12401240
if (NewExpr.isInvalid())
12411241
return false;
12421242

@@ -1262,8 +1262,7 @@ bool CoroutineStmtBuilder::makeNewAndDeleteExpr() {
12621262

12631263
ExprResult DeleteExpr =
12641264
S.ActOnCallExpr(S.getCurScope(), DeleteRef.get(), Loc, DeleteArgs, Loc);
1265-
DeleteExpr =
1266-
S.ActOnFinishFullExpr(DeleteExpr.get(), /*DiscardedValue*/ false);
1265+
DeleteExpr = S.ActOnFinishFullExpr(DeleteExpr.get());
12671266
if (DeleteExpr.isInvalid())
12681267
return false;
12691268

@@ -1348,8 +1347,7 @@ bool CoroutineStmtBuilder::makeOnException() {
13481347

13491348
ExprResult UnhandledException = buildPromiseCall(S, Fn.CoroutinePromise, Loc,
13501349
"unhandled_exception", None);
1351-
UnhandledException = S.ActOnFinishFullExpr(UnhandledException.get(), Loc,
1352-
/*DiscardedValue*/ false);
1350+
UnhandledException = S.ActOnFinishFullExpr(UnhandledException.get(), Loc);
13531351
if (UnhandledException.isInvalid())
13541352
return false;
13551353

@@ -1402,8 +1400,7 @@ bool CoroutineStmtBuilder::makeGroDeclAndReturnStmt() {
14021400
"get_return_object type must no longer be dependent");
14031401

14041402
if (FnRetType->isVoidType()) {
1405-
ExprResult Res =
1406-
S.ActOnFinishFullExpr(this->ReturnValue, Loc, /*DiscardedValue*/ false);
1403+
ExprResult Res = S.ActOnFinishFullExpr(this->ReturnValue, Loc);
14071404
if (Res.isInvalid())
14081405
return false;
14091406

@@ -1435,7 +1432,7 @@ bool CoroutineStmtBuilder::makeGroDeclAndReturnStmt() {
14351432
if (Res.isInvalid())
14361433
return false;
14371434

1438-
Res = S.ActOnFinishFullExpr(Res.get(), /*DiscardedValue*/ false);
1435+
Res = S.ActOnFinishFullExpr(Res.get());
14391436
if (Res.isInvalid())
14401437
return false;
14411438

‎clang/lib/Sema/SemaDecl.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11200,9 +11200,9 @@ void Sema::AddInitializerToDecl(Decl *RealDecl, Expr *Init, bool DirectInit) {
1120011200
// struct T { S a, b; } t = { Temp(), Temp() }
1120111201
//
1120211202
// we should destroy the first Temp before constructing the second.
11203-
ExprResult Result =
11204-
ActOnFinishFullExpr(Init, VDecl->getLocation(),
11205-
/*DiscardedValue*/ false, VDecl->isConstexpr());
11203+
ExprResult Result = ActOnFinishFullExpr(Init, VDecl->getLocation(),
11204+
false,
11205+
VDecl->isConstexpr());
1120611206
if (Result.isInvalid()) {
1120711207
VDecl->setInvalidDecl();
1120811208
return;

‎clang/lib/Sema/SemaDeclCXX.cpp

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1205,7 +1205,7 @@ static bool checkTupleLikeDecomposition(Sema &S,
12051205
E = Seq.Perform(S, Entity, Kind, Init);
12061206
if (E.isInvalid())
12071207
return true;
1208-
E = S.ActOnFinishFullExpr(E.get(), Loc, /*DiscardedValue*/ false);
1208+
E = S.ActOnFinishFullExpr(E.get(), Loc);
12091209
if (E.isInvalid())
12101210
return true;
12111211
RefVD->setInit(E.get());
@@ -3686,7 +3686,7 @@ void Sema::ActOnFinishCXXInClassMemberInitializer(Decl *D,
36863686
// C++11 [class.base.init]p7:
36873687
// The initialization of each base and member constitutes a
36883688
// full-expression.
3689-
Init = ActOnFinishFullExpr(Init.get(), InitLoc, /*DiscardedValue*/ false);
3689+
Init = ActOnFinishFullExpr(Init.get(), InitLoc);
36903690
if (Init.isInvalid()) {
36913691
FD->setInvalidDecl();
36923692
return;
@@ -4044,8 +4044,7 @@ Sema::BuildMemberInitializer(ValueDecl *Member, Expr *Init,
40444044
// C++11 [class.base.init]p7:
40454045
// The initialization of each base and member constitutes a
40464046
// full-expression.
4047-
MemberInit = ActOnFinishFullExpr(MemberInit.get(), InitRange.getBegin(),
4048-
/*DiscardedValue*/ false);
4047+
MemberInit = ActOnFinishFullExpr(MemberInit.get(), InitRange.getBegin());
40494048
if (MemberInit.isInvalid())
40504049
return true;
40514050

@@ -4100,8 +4099,8 @@ Sema::BuildDelegatingInitializer(TypeSourceInfo *TInfo, Expr *Init,
41004099
// C++11 [class.base.init]p7:
41014100
// The initialization of each base and member constitutes a
41024101
// full-expression.
4103-
DelegationInit = ActOnFinishFullExpr(
4104-
DelegationInit.get(), InitRange.getBegin(), /*DiscardedValue*/ false);
4102+
DelegationInit = ActOnFinishFullExpr(DelegationInit.get(),
4103+
InitRange.getBegin());
41054104
if (DelegationInit.isInvalid())
41064105
return true;
41074106

@@ -4230,8 +4229,7 @@ Sema::BuildBaseInitializer(QualType BaseType, TypeSourceInfo *BaseTInfo,
42304229
// C++11 [class.base.init]p7:
42314230
// The initialization of each base and member constitutes a
42324231
// full-expression.
4233-
BaseInit = ActOnFinishFullExpr(BaseInit.get(), InitRange.getBegin(),
4234-
/*DiscardedValue*/ false);
4232+
BaseInit = ActOnFinishFullExpr(BaseInit.get(), InitRange.getBegin());
42354233
if (BaseInit.isInvalid())
42364234
return true;
42374235

‎clang/lib/Sema/SemaExpr.cpp

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4723,9 +4723,8 @@ bool Sema::CheckCXXDefaultArgExpr(SourceLocation CallLoc, FunctionDecl *FD,
47234723
if (Result.isInvalid())
47244724
return true;
47254725

4726-
Result =
4727-
ActOnFinishFullExpr(Result.getAs<Expr>(), Param->getOuterLocStart(),
4728-
/*DiscardedValue*/ false);
4726+
Result = ActOnFinishFullExpr(Result.getAs<Expr>(),
4727+
Param->getOuterLocStart());
47294728
if (Result.isInvalid())
47304729
return true;
47314730

‎clang/lib/Sema/SemaExprCXX.cpp

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7815,8 +7815,6 @@ ExprResult Sema::ActOnFinishFullExpr(Expr *FE, SourceLocation CC,
78157815
FullExpr = IgnoredValueConversions(FullExpr.get());
78167816
if (FullExpr.isInvalid())
78177817
return ExprError();
7818-
7819-
DiagnoseUnusedExprResult(FullExpr.get());
78207818
}
78217819

78227820
FullExpr = CorrectDelayedTyposInExpr(FullExpr.get());

0 commit comments

Comments
 (0)