[Clangd] Ensure children are always RootStmt in ExtractFunction (Fixes #153)
Summary:
We weren't always checking if children are RootStmts in ExtractFunction.
For `void f([[int a]]);`, the ParmVarDecl appeared as a RootStmt since
we didn't perform the check and ended up being casted to a (null) Stmt.
Reviewers: sammccall, kadircet
Subscribers: kristof.beyls, ilya-biryukov, MaskRay, jkorous, arphaman, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D68182
git-svn-id: https://llvm.org/svn/llvm-project/clang-tools-extra/trunk@373471 91177308-0d34-0410-b5e6-96231b3b80d8
diff --git a/clangd/refactor/tweaks/ExtractFunction.cpp b/clangd/refactor/tweaks/ExtractFunction.cpp
index e9f617e..8c2403a 100644
--- a/clangd/refactor/tweaks/ExtractFunction.cpp
+++ b/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -104,6 +104,11 @@
}
// Returns the (unselected) parent of all RootStmts given the commonAncestor.
+// Returns null if:
+// 1. any node is partially selected
+// 2. If all completely selected nodes don't have the same common parent
+// 3. Any child of Parent isn't a RootStmt.
+// Returns null if any child is not a RootStmt.
// We only support extraction of RootStmts since it allows us to extract without
// having to change the selection range. Also, this means that any scope that
// begins in selection range, ends in selection range and any scope that begins
@@ -111,25 +116,30 @@
const Node *getParentOfRootStmts(const Node *CommonAnc) {
if (!CommonAnc)
return nullptr;
+ const Node *Parent = nullptr;
switch (CommonAnc->Selected) {
case SelectionTree::Selection::Unselected:
// Typicaly a block, with the { and } unselected, could also be ForStmt etc
// Ensure all Children are RootStmts.
- return llvm::all_of(CommonAnc->Children, isRootStmt) ? CommonAnc : nullptr;
+ Parent = CommonAnc;
+ break;
case SelectionTree::Selection::Partial:
// Only a fully-selected single statement can be selected.
return nullptr;
case SelectionTree::Selection::Complete:
// If the Common Ancestor is completely selected, then it's a root statement
// and its parent will be unselected.
- const Node *Parent = CommonAnc->Parent;
+ Parent = CommonAnc->Parent;
// If parent is a DeclStmt, even though it's unselected, we consider it a
// root statement and return its parent. This is done because the VarDecls
// claim the entire selection range of the Declaration and DeclStmt is
// always unselected.
- return Parent->ASTNode.get<DeclStmt>() ? Parent->Parent : Parent;
+ if (Parent->ASTNode.get<DeclStmt>())
+ Parent = Parent->Parent;
+ break;
}
- llvm_unreachable("Unhandled SelectionTree::Selection enum");
+ // Ensure all Children are RootStmts.
+ return llvm::all_of(Parent->Children, isRootStmt) ? Parent : nullptr;
}
// The ExtractionZone class forms a view of the code wrt Zone.
diff --git a/clangd/unittests/TweakTests.cpp b/clangd/unittests/TweakTests.cpp
index 6234fb7..2c4aaa5 100644
--- a/clangd/unittests/TweakTests.cpp
+++ b/clangd/unittests/TweakTests.cpp
@@ -629,6 +629,9 @@
F ([[int x = 0;]])
)cpp";
EXPECT_EQ(apply(MacroFailInput), "unavailable");
+
+ // Shouldn't crash.
+ EXPECT_EQ(apply("void f([[int a]]);"), "unavailable");
}
TEST_F(ExtractFunctionTest, ControlFlow) {