SureYeaah updated this revision to Diff 217881.
SureYeaah added a comment.
Added null statement check in TraverseStmt
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66732/new/
https://reviews.llvm.org/D66732
Files:
clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
clang-tools-extra/clangd/unittests/TweakTests.cpp
Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -520,14 +520,22 @@
// FIXME: ExtractFunction should be unavailable inside loop construct
// initalizer/condition.
EXPECT_THAT(apply(" for([[int i = 0;]];);"), HasSubstr("extracted"));
+ // We should be able to extract break/continue with a parent loop/switch.
+ EXPECT_THAT(apply(" [[for(;;) if(1) break;]] "), HasSubstr("extracted"));
+ EXPECT_THAT(apply(" for(;;) [[while(1) break;]] "), HasSubstr("extracted"));
+ EXPECT_THAT(apply(" [[switch(1) { break; }]]"), HasSubstr("extracted"));
+ EXPECT_THAT(apply(" [[while(1) switch(1) { continue; }]]"),
+ HasSubstr("extracted"));
// Don't extract because needs hoisting.
EXPECT_THAT(apply(" [[int a = 5;]] a++; "), StartsWith("fail"));
// Don't extract return
EXPECT_THAT(apply(" if(true) [[return;]] "), StartsWith("fail"));
- // Don't extract break and continue.
- // FIXME: We should be able to extract this since it's non broken.
- EXPECT_THAT(apply(" [[for(;;) break;]] "), StartsWith("fail"));
- EXPECT_THAT(apply(" for(;;) [[continue;]] "), StartsWith("fail"));
+ // Don't extract break and continue without a loop/switch parent.
+ EXPECT_THAT(apply(" for(;;) [[if(1) continue;]] "), StartsWith("fail"));
+ EXPECT_THAT(apply(" while(1) [[if(1) break;]] "), StartsWith("fail"));
+ EXPECT_THAT(apply(" switch(1) { [[break;]] }"), StartsWith("fail"));
+ EXPECT_THAT(apply(" for(;;) { [[while(1) break; break;]] }"),
+ StartsWith("fail"));
}
TEST_F(ExtractFunctionTest, FileTest) {
Index: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
===================================================================
--- clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -29,7 +29,7 @@
// - Void return type
// - Cannot extract declarations that will be needed in the original function
// after extraction.
-// - Doesn't check for broken control flow (break/continue without loop/switch)
+// - Checks for broken control flow (break/continue without loop/switch)
//
// 1. ExtractFunction is the tweak subclass
// - Prepare does basic analysis of the selection and is therefore fast.
@@ -152,6 +152,7 @@
// semicolon after the extraction.
const Node *getLastRootStmt() const { return Parent->Children.back(); }
void generateRootStmts();
+
private:
llvm::DenseSet<const Stmt *> RootStmts;
};
@@ -162,7 +163,7 @@
// Generate RootStmts set
void ExtractionZone::generateRootStmts() {
- for(const Node *Child : Parent->Children)
+ for (const Node *Child : Parent->Children)
RootStmts.insert(Child->ASTNode.get<Stmt>());
}
@@ -178,7 +179,7 @@
if (isa<CXXMethodDecl>(Func))
return nullptr;
// FIXME: Support extraction from templated functions.
- if(Func->isTemplated())
+ if (Func->isTemplated())
return nullptr;
return Func;
}
@@ -350,8 +351,9 @@
llvm::DenseMap<const Decl *, DeclInformation> DeclInfoMap;
// True if there is a return statement in zone.
bool HasReturnStmt = false;
- // For now we just care whether there exists a break/continue in zone.
- bool HasBreakOrContinue = false;
+ // Control flow is broken if we are extracting a break/continue without a
+ // corresponding parent loop/switch
+ bool BrokenControlFlow = false;
// FIXME: capture TypeAliasDecl and UsingDirectiveDecl
// FIXME: Capture type information as well.
DeclInformation *createDeclInfo(const Decl *D, ZoneRelative RelativeLoc);
@@ -390,6 +392,11 @@
}
}
+bool isLoop(const Stmt *S) {
+ return isa<ForStmt>(S) || isa<DoStmt>(S) || isa<WhileStmt>(S) ||
+ isa<CXXForRangeStmt>(S);
+}
+
// Captures information from Extraction Zone
CapturedZoneInfo captureZoneInfo(const ExtractionZone &ExtZone) {
// We use the ASTVisitor instead of using the selection tree since we need to
@@ -401,24 +408,53 @@
ExtractionZoneVisitor(const ExtractionZone &ExtZone) : ExtZone(ExtZone) {
TraverseDecl(const_cast<FunctionDecl *>(ExtZone.EnclosingFunction));
}
+
bool TraverseStmt(Stmt *S) {
+ if (!S)
+ return true;
bool IsRootStmt = ExtZone.isRootStmt(const_cast<const Stmt *>(S));
// If we are starting traversal of a RootStmt, we are somewhere inside
// ExtractionZone
if (IsRootStmt)
CurrentLocation = ZoneRelative::Inside;
+ incrementLoopSwitchCounters(S);
// Traverse using base class's TraverseStmt
RecursiveASTVisitor::TraverseStmt(S);
+ decrementLoopSwitchCounters(S);
// We set the current location as after since next stmt will either be a
// RootStmt (handled at the beginning) or after extractionZone
if (IsRootStmt)
CurrentLocation = ZoneRelative::After;
return true;
}
+
+ // Increment CurNumberOf{Loops,Switch} if statement is {Loop,Switch} and
+ // inside Extraction Zone.
+ void incrementLoopSwitchCounters(Stmt *S) {
+ if (CurrentLocation != ZoneRelative::Inside)
+ return;
+ if (isLoop(S))
+ CurNumberOfLoops++;
+ else if (isa<SwitchStmt>(S))
+ CurNumberOfSwitch++;
+ }
+
+ // Decrement CurNumberOf{Loops,Switch} if statement is {Loop,Switch} and
+ // inside Extraction Zone.
+ void decrementLoopSwitchCounters(Stmt *S) {
+ if (CurrentLocation != ZoneRelative::Inside)
+ return;
+ if (isLoop(S))
+ CurNumberOfLoops--;
+ else if (isa<SwitchStmt>(S))
+ CurNumberOfSwitch--;
+ }
+
bool VisitDecl(Decl *D) {
Info.createDeclInfo(D, CurrentLocation);
return true;
}
+
bool VisitDeclRefExpr(DeclRefExpr *DRE) {
// Find the corresponding Decl and mark it's occurence.
const Decl *D = DRE->getDecl();
@@ -430,26 +466,36 @@
// FIXME: check if reference mutates the Decl being referred.
return true;
}
+
bool VisitReturnStmt(ReturnStmt *Return) {
if (CurrentLocation == ZoneRelative::Inside)
Info.HasReturnStmt = true;
return true;
}
- // FIXME: check for broken break/continue only.
bool VisitBreakStmt(BreakStmt *Break) {
- if (CurrentLocation == ZoneRelative::Inside)
- Info.HasBreakOrContinue = true;
+ // Control flow is broken if break statement is selected without any
+ // parent loop or switch statement.
+ if (CurrentLocation == ZoneRelative::Inside &&
+ !(CurNumberOfLoops + CurNumberOfSwitch))
+ Info.BrokenControlFlow = true;
return true;
}
+
bool VisitContinueStmt(ContinueStmt *Continue) {
- if (CurrentLocation == ZoneRelative::Inside)
- Info.HasBreakOrContinue = true;
+ // Control flow is broken if Continue statement is selected without any
+ // parent loop
+ if (CurrentLocation == ZoneRelative::Inside && !CurNumberOfLoops)
+ Info.BrokenControlFlow = true;
return true;
}
CapturedZoneInfo Info;
const ExtractionZone &ExtZone;
ZoneRelative CurrentLocation = ZoneRelative::Before;
+ // Number of {loop,switch} statements that are currently in the traversal
+ // stack inside Extraction Zone. Used to check for broken control flow.
+ unsigned CurNumberOfLoops = 0;
+ unsigned CurNumberOfSwitch = 0;
};
ExtractionZoneVisitor Visitor(ExtZone);
return std::move(Visitor.Info);
@@ -531,10 +577,10 @@
const LangOptions &LangOpts) {
CapturedZoneInfo CapturedInfo = captureZoneInfo(ExtZone);
// Bail out if any break of continue exists
- // FIXME: check for broken control flow only
- if (CapturedInfo.HasBreakOrContinue)
+ if (CapturedInfo.BrokenControlFlow)
return llvm::createStringError(llvm::inconvertibleErrorCode(),
- +"Cannot extract break or continue.");
+ +"Cannot extract break/continue without "
+ "corresponding loop/switch statement.");
NewFunction ExtractedFunc(getSemicolonPolicy(ExtZone, SM, LangOpts));
ExtractedFunc.BodyRange = ExtZone.ZoneRange;
ExtractedFunc.InsertionPoint = ExtZone.getInsertionPoint();
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
