Skip to content

Commit e6ec9b0

Browse files
hamishknighthamishknight
authored
Merge pull request #70172 from hamishknight/or-else
2 parents d54f03e + c3b055a commit e6ec9b0

19 files changed

+333
-126
lines changed

‎include/swift/AST/ASTBridging.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -734,7 +734,8 @@ SWIFT_NAME("BridgedIfStmt.createParsed(_:ifKeywordLoc:condition:thenStmt:"
734734
"elseLoc:elseStmt:)")
735735
BridgedIfStmt BridgedIfStmt_createParsed(BridgedASTContext cContext,
736736
BridgedSourceLoc cIfLoc,
737-
BridgedExpr cond, BridgedStmt then,
737+
BridgedExpr cond,
738+
BridgedBraceStmt then,
738739
BridgedSourceLoc cElseLoc,
739740
BridgedNullableStmt elseStmt);
740741

‎include/swift/AST/Stmt.h

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -757,20 +757,25 @@ class LabeledConditionalStmt : public LabeledStmt {
757757
class IfStmt : public LabeledConditionalStmt {
758758
SourceLoc IfLoc;
759759
SourceLoc ElseLoc;
760-
Stmt *Then;
760+
BraceStmt *Then;
761761
Stmt *Else;
762762

763763
public:
764764
IfStmt(LabeledStmtInfo LabelInfo, SourceLoc IfLoc, StmtCondition Cond,
765-
Stmt *Then, SourceLoc ElseLoc, Stmt *Else,
765+
BraceStmt *Then, SourceLoc ElseLoc, Stmt *Else,
766766
llvm::Optional<bool> implicit = llvm::None)
767767
: LabeledConditionalStmt(StmtKind::If,
768768
getDefaultImplicitFlag(implicit, IfLoc),
769769
LabelInfo, Cond),
770-
IfLoc(IfLoc), ElseLoc(ElseLoc), Then(Then), Else(Else) {}
770+
IfLoc(IfLoc), ElseLoc(ElseLoc), Then(Then), Else(Else) {
771+
assert(Then && "Must have non-null 'then' statement");
772+
assert(!Else || isa<BraceStmt>(Else) ||
773+
isa<IfStmt>(Else) &&
774+
"Else statement must either be BraceStmt or IfStmt");
775+
}
771776

772-
IfStmt(SourceLoc IfLoc, Expr *Cond, Stmt *Then, SourceLoc ElseLoc, Stmt *Else,
773-
llvm::Optional<bool> implicit, ASTContext &Ctx);
777+
IfStmt(SourceLoc IfLoc, Expr *Cond, BraceStmt *Then, SourceLoc ElseLoc,
778+
Stmt *Else, llvm::Optional<bool> implicit, ASTContext &Ctx);
774779

775780
SourceLoc getIfLoc() const { return IfLoc; }
776781
SourceLoc getElseLoc() const { return ElseLoc; }
@@ -782,8 +787,8 @@ class IfStmt : public LabeledConditionalStmt {
782787
return (Else ? Else->getEndLoc() : Then->getEndLoc());
783788
}
784789

785-
Stmt *getThenStmt() const { return Then; }
786-
void setThenStmt(Stmt *s) { Then = s; }
790+
BraceStmt *getThenStmt() const { return Then; }
791+
void setThenStmt(BraceStmt *s) { Then = s; }
787792

788793
Stmt *getElseStmt() const { return Else; }
789794
void setElseStmt(Stmt *s) { Else = s; }

‎lib/AST/ASTBridging.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1051,7 +1051,8 @@ BridgedBraceStmt BridgedBraceStmt_createParsed(BridgedASTContext cContext,
10511051

10521052
BridgedIfStmt BridgedIfStmt_createParsed(BridgedASTContext cContext,
10531053
BridgedSourceLoc cIfLoc,
1054-
BridgedExpr cond, BridgedStmt then,
1054+
BridgedExpr cond,
1055+
BridgedBraceStmt then,
10551056
BridgedSourceLoc cElseLoc,
10561057
BridgedNullableStmt elseStmt) {
10571058
ASTContext &context = cContext.unbridged();

‎lib/AST/ASTWalker.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1813,8 +1813,8 @@ Stmt *Traversal::visitIfStmt(IfStmt *IS) {
18131813
if (doIt(IS->getCond()))
18141814
return nullptr;
18151815

1816-
if (Stmt *S2 = doIt(IS->getThenStmt()))
1817-
IS->setThenStmt(S2);
1816+
if (auto *S2 = doIt(IS->getThenStmt()))
1817+
IS->setThenStmt(cast<BraceStmt>(S2));
18181818
else
18191819
return nullptr;
18201820

‎lib/AST/Stmt.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -661,7 +661,7 @@ static StmtCondition exprToCond(Expr *C, ASTContext &Ctx) {
661661
return Ctx.AllocateCopy(Arr);
662662
}
663663

664-
IfStmt::IfStmt(SourceLoc IfLoc, Expr *Cond, Stmt *Then, SourceLoc ElseLoc,
664+
IfStmt::IfStmt(SourceLoc IfLoc, Expr *Cond, BraceStmt *Then, SourceLoc ElseLoc,
665665
Stmt *Else, llvm::Optional<bool> implicit, ASTContext &Ctx)
666666
: IfStmt(LabeledStmtInfo(), IfLoc, exprToCond(Cond, Ctx), Then, ElseLoc,
667667
Else, implicit) {}

‎lib/ASTGen/Sources/ASTGen/Stmts.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ extension ASTGenVisitor {
7373
self.ctx,
7474
ifKeywordLoc: node.ifKeyword.bridgedSourceLoc(in: self),
7575
condition: conditions.first!.castToExpr,
76-
thenStmt: self.generate(codeBlock: node.body).asStmt,
76+
thenStmt: self.generate(codeBlock: node.body),
7777
elseLoc: node.elseKeyword.bridgedSourceLoc(in: self),
7878
elseStmt: node.elseBody.map {
7979
switch $0 {

‎lib/SIL/IR/SILProfiler.cpp

Lines changed: 80 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -422,6 +422,13 @@ class CounterExpr {
422422
/// Returns true if this is a zero counter.
423423
bool isZero() const { return Counter.isZero(); }
424424

425+
friend bool operator==(const CounterExpr &LHS, const CounterExpr &RHS) {
426+
return LHS.Counter == RHS.Counter;
427+
}
428+
friend bool operator!=(const CounterExpr &LHS, const CounterExpr &RHS) {
429+
return !(LHS == RHS);
430+
}
431+
425432
llvm::coverage::Counter getLLVMCounter() const { return Counter; }
426433

427434
void print(raw_ostream &OS,
@@ -476,8 +483,9 @@ class SourceMappingRegion {
476483
}
477484

478485
SourceMappingRegion(Kind RegionKind, ASTNode Node, SourceRange Range,
486+
llvm::Optional<CounterExpr> Counter,
479487
const SourceManager &SM)
480-
: RegionKind(RegionKind), Node(Node) {
488+
: RegionKind(RegionKind), Node(Node), Counter(Counter) {
481489
assert(Range.isValid());
482490
StartLoc = Range.Start;
483491
EndLoc = Lexer::getLocForEndOfToken(SM, Range.End);
@@ -492,16 +500,18 @@ class SourceMappingRegion {
492500

493501
// Note we don't store counters for nodes, as we need to be able to fix them
494502
// up later.
495-
return SourceMappingRegion(Kind::Node, Node, Range, SM);
503+
return SourceMappingRegion(Kind::Node, Node, Range, /*Counter*/ llvm::None,
504+
SM);
496505
}
497506

498507
/// Create a source region for an ASTNode that is only present for scoping of
499508
/// child regions, and doesn't need to be included in the resulting set of
500509
/// regions.
501-
static SourceMappingRegion scopingOnly(ASTNode Node,
502-
const SourceManager &SM) {
510+
static SourceMappingRegion
511+
scopingOnly(ASTNode Node, const SourceManager &SM,
512+
llvm::Optional<CounterExpr> Counter = llvm::None) {
503513
return SourceMappingRegion(Kind::ScopingOnly, Node, Node.getSourceRange(),
504-
SM);
514+
Counter, SM);
505515
}
506516

507517
/// Create a refined region for a given counter.
@@ -967,6 +977,14 @@ struct CoverageMapping : public ASTWalker {
967977
return;
968978
}
969979

980+
// Don't apply exit adjustments to if statement branches, they should
981+
// be handled at the end of the statement. This avoids creating awkward
982+
// overlapping exit regions for each branch, and ensures 'break'
983+
// statements only have their jump counted once for the entire
984+
// statement.
985+
if (isa<IfStmt>(ParentStmt))
986+
return;
987+
970988
if (auto *LS = dyn_cast<LabeledStmt>(ParentStmt))
971989
JumpsToLabel = getCounter(LS);
972990
}
@@ -1225,7 +1243,7 @@ struct CoverageMapping : public ASTWalker {
12251243
// it by break statements.
12261244
assignCounter(IS, CounterExpr::Zero());
12271245

1228-
// FIXME: This is a redundant region.
1246+
// FIXME: This is a redundant region for non else-ifs.
12291247
if (auto *Cond = getConditionNode(IS->getCond()))
12301248
assignCounter(Cond, getCurrentCounter());
12311249

@@ -1244,13 +1262,68 @@ struct CoverageMapping : public ASTWalker {
12441262
// terms of it.
12451263
auto ThenCounter = assignKnownCounter(IS->getThenStmt());
12461264
IS->getThenStmt()->walk(*this);
1265+
auto ThenDelta =
1266+
CounterExpr::Sub(ThenCounter, getExitCounter(), CounterBuilder);
12471267

1268+
llvm::Optional<CounterExpr> ElseDelta;
12481269
if (auto *Else = IS->getElseStmt()) {
12491270
auto ElseCounter = CounterExpr::Sub(ParentCounter, ThenCounter,
12501271
CounterBuilder);
1251-
assignCounter(Else, ElseCounter);
1272+
// We handle `else if` and `else` slightly differently here. For
1273+
// `else` we have a BraceStmt, and can use the existing scoping logic
1274+
// to handle calculating the exit count. For `else if`, we need to
1275+
// set up a new scope to contain the child `if` statement, effectively
1276+
// we treat:
1277+
//
1278+
// if .random() {
1279+
// } else if .random() {
1280+
// } else {
1281+
// }
1282+
//
1283+
// the same as:
1284+
//
1285+
// if .random() {
1286+
// } else {
1287+
// if .random() {
1288+
// } else {
1289+
// }
1290+
// }
1291+
//
1292+
// This ensures we assign a correct counter to the `else if`
1293+
// condition, and allows us to compute the exit count correctly. We
1294+
// don't need the fake `else` scope to be included in the resulting
1295+
// set of regions, so we mark it scoping-only.
1296+
if (isa<BraceStmt>(Else)) {
1297+
assignCounter(Else, ElseCounter);
1298+
} else {
1299+
pushRegion(SourceMappingRegion::scopingOnly(Else, SM, ElseCounter));
1300+
}
12521301
Else->walk(*this);
1302+
1303+
// Once we've walked the `else`, compute the delta exit count. For
1304+
// a normal `else` we can use the computed exit count, for an
1305+
// `else if` we can take the current region count since we don't have
1306+
// a proper scope. This is a little hacked together, but we'll be able
1307+
// to do away with all of this once we re-implement as a SILOptimizer
1308+
// pass.
1309+
auto AfterElse = isa<BraceStmt>(Else) ? getExitCounter()
1310+
: getCurrentCounter();
1311+
if (!isa<BraceStmt>(Else))
1312+
popRegions(Else);
1313+
1314+
ElseDelta = CounterExpr::Sub(ElseCounter, AfterElse, CounterBuilder);
12531315
}
1316+
// Compute the exit count following the `if`, taking jumps to the
1317+
// statement by breaks into account, and the delta of the `then` branch
1318+
// and `else` branch if we have one.
1319+
auto AfterIf = getCurrentCounter();
1320+
AfterIf = CounterExpr::Add(AfterIf, getCounter(IS), CounterBuilder);
1321+
AfterIf = CounterExpr::Sub(AfterIf, ThenDelta, CounterBuilder);
1322+
if (ElseDelta)
1323+
AfterIf = CounterExpr::Sub(AfterIf, *ElseDelta, CounterBuilder);
1324+
1325+
if (AfterIf != getCurrentCounter())
1326+
replaceCount(AfterIf, getEndLoc(IS));
12541327
}
12551328
// Already visited the children.
12561329
// FIXME: The ASTWalker should do a post-walk for SkipChildren.

‎lib/Sema/BuilderTransform.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -494,7 +494,7 @@ class ResultBuilderTransform
494494
availabilityCond && builder.supports(ctx.Id_buildLimitedAvailability);
495495

496496
NullablePtr<Expr> thenVarRef;
497-
NullablePtr<Stmt> thenBranch;
497+
NullablePtr<BraceStmt> thenBranch;
498498
{
499499
SmallVector<ASTNode, 4> thenBody;
500500

‎lib/Sema/CSSyntacticElement.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1803,7 +1803,7 @@ class SyntacticElementSolutionApplication
18031803
else
18041804
hadError = true;
18051805

1806-
ifStmt->setThenStmt(visit(ifStmt->getThenStmt()).get<Stmt *>());
1806+
ifStmt->setThenStmt(castToStmt<BraceStmt>(visit(ifStmt->getThenStmt())));
18071807

18081808
if (auto elseStmt = ifStmt->getElseStmt()) {
18091809
ifStmt->setElseStmt(visit(elseStmt).get<Stmt *>());

‎lib/Sema/PCMacro.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -142,10 +142,10 @@ class Instrumenter : InstrumenterBase {
142142
transformStmtCondition(SC, IS->getStartLoc());
143143
IS->setCond(SC); // FIXME: is setting required?..
144144

145-
if (Stmt *TS = IS->getThenStmt()) {
146-
Stmt *NTS = transformStmt(TS);
145+
if (auto *TS = IS->getThenStmt()) {
146+
auto *NTS = transformStmt(TS);
147147
if (NTS != TS) {
148-
IS->setThenStmt(NTS);
148+
IS->setThenStmt(cast<BraceStmt>(NTS));
149149
}
150150
}
151151

‎lib/Sema/PlaygroundTransform.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -205,10 +205,10 @@ class Instrumenter : InstrumenterBase {
205205
// transform*() return their input if it's unmodified,
206206
// or a modified copy of their input otherwise.
207207
IfStmt *transformIfStmt(IfStmt *IS) {
208-
if (Stmt *TS = IS->getThenStmt()) {
209-
Stmt *NTS = transformStmt(TS);
208+
if (auto *TS = IS->getThenStmt()) {
209+
auto *NTS = transformStmt(TS);
210210
if (NTS != TS) {
211-
IS->setThenStmt(NTS);
211+
IS->setThenStmt(cast<BraceStmt>(NTS));
212212
}
213213
}
214214

‎lib/Sema/TypeCheckStmt.cpp

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1377,15 +1377,15 @@ class StmtChecker : public StmtVisitor<StmtChecker, Stmt*> {
13771377
auto sourceFile = DC->getParentSourceFile();
13781378
checkLabeledStmtShadowing(getASTContext(), sourceFile, IS);
13791379

1380-
Stmt *S = IS->getThenStmt();
1381-
typeCheckStmt(S);
1382-
IS->setThenStmt(S);
1380+
auto *TS = IS->getThenStmt();
1381+
typeCheckStmt(TS);
1382+
IS->setThenStmt(TS);
13831383

1384-
if ((S = IS->getElseStmt())) {
1385-
typeCheckStmt(S);
1386-
IS->setElseStmt(S);
1384+
if (auto *ES = IS->getElseStmt()) {
1385+
typeCheckStmt(ES);
1386+
IS->setElseStmt(ES);
13871387
}
1388-
1388+
13891389
return IS;
13901390
}
13911391

‎lib/Sema/TypeCheckStorage.cpp

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1641,14 +1641,13 @@ synthesizeLazyGetterBody(AccessorDecl *Get, VarDecl *VD, VarDecl *Storage,
16411641
Tmp1DRE->setType(Tmp1VD->getTypeInContext());
16421642
auto *Return = new (Ctx) ReturnStmt(SourceLoc(), Tmp1DRE,
16431643
/*implicit*/true);
1644-
1644+
auto *ReturnBranch = BraceStmt::createImplicit(Ctx, {Return});
16451645

16461646
// Build the "if" around the early return.
1647-
Body.push_back(new (Ctx) IfStmt(LabeledStmtInfo(),
1648-
SourceLoc(), Ctx.AllocateCopy(Cond), Return,
1649-
/*elseloc*/SourceLoc(), /*else*/nullptr,
1650-
/*implicit*/ true));
1651-
1647+
Body.push_back(new (Ctx) IfStmt(LabeledStmtInfo(), SourceLoc(),
1648+
Ctx.AllocateCopy(Cond), ReturnBranch,
1649+
/*elseloc*/ SourceLoc(),
1650+
/*else*/ nullptr, /*implicit*/ true));
16521651

16531652
auto *Tmp2VD = new (Ctx) VarDecl(/*IsStatic*/false, VarDecl::Introducer::Let,
16541653
SourceLoc(), Ctx.getIdentifier("tmp2"),

‎test/Profiler/coverage_deinit.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,4 +21,4 @@ public class Foo {
2121
// CHECK-NEXT: [[@LINE-8]]:10 -> [[@LINE-4]]:4 : 0
2222
// CHECK-NEXT: [[@LINE-8]]:8 -> [[@LINE-8]]:17 : 0
2323
// CHECK-NEXT: [[@LINE-9]]:18 -> [[@LINE-7]]:6 : 1
24-
// CHECK-NEXT: [[@LINE-8]]:6 -> [[@LINE-7]]:4 : 0
24+
// CHECK-NEXT: }

0 commit comments

Comments
 (0)