[clang-tools-extra] r373442 - [clangd] SelectionTree should mark a node as fully-selected if the only claimed tokens were early-claimed.
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Wed Oct 2 03:01:53 PDT 2019
Author: sammccall
Date: Wed Oct 2 03:01:53 2019
New Revision: 373442
URL: http://llvm.org/viewvc/llvm-project?rev=373442&view=rev
Log:
[clangd] SelectionTree should mark a node as fully-selected if the only claimed tokens were early-claimed.
Summary:
Previously they would be marked as partially-selected based on the early claim,
and never updated as no more tokens were claimed.
This affects simple VarDecls like "int x".
Reviewers: SureYeaah
Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D66872
Modified:
clang-tools-extra/trunk/clangd/Selection.cpp
clang-tools-extra/trunk/clangd/refactor/tweaks/ExtractFunction.cpp
clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp
clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp
Modified: clang-tools-extra/trunk/clangd/Selection.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Selection.cpp?rev=373442&r1=373441&r2=373442&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/Selection.cpp (original)
+++ clang-tools-extra/trunk/clangd/Selection.cpp Wed Oct 2 03:01:53 2019
@@ -61,13 +61,13 @@ public:
// Associates any tokens overlapping [Begin, End) with an AST node.
// Tokens that were already claimed by another AST node are not claimed again.
- // Returns whether the node is selected in the sense of SelectionTree.
- SelectionTree::Selection claim(unsigned Begin, unsigned End) {
+ // Updates Result if the node is selected in the sense of SelectionTree.
+ void claim(unsigned Begin, unsigned End, SelectionTree::Selection &Result) {
assert(Begin <= End);
// Fast-path for missing the selection entirely.
if (Begin >= SelEnd || End <= SelBegin)
- return SelectionTree::Unselected;
+ return;
// We will consider the range (at least partially) selected if it hit any
// selected and previously unclaimed token.
@@ -98,9 +98,13 @@ public:
}
}
- if (!ClaimedAnyToken)
- return SelectionTree::Unselected;
- return PartialSelection ? SelectionTree::Partial : SelectionTree::Complete;
+ // If some tokens were previously claimed (Result != Unselected), we may
+ // upgrade from Partial->Complete, even if no new tokens were claimed.
+ // Important for [[int a]].
+ if (ClaimedAnyToken || Result) {
+ Result = std::max(Result, PartialSelection ? SelectionTree::Partial
+ : SelectionTree::Complete);
+ }
}
private:
@@ -162,7 +166,9 @@ public:
assert(V.Stack.size() == 1 && "Unpaired push/pop?");
assert(V.Stack.top() == &V.Nodes.front());
// We selected TUDecl if tokens were unclaimed (or the file is empty).
- if (V.Nodes.size() == 1 || V.Claimed.claim(Begin, End)) {
+ SelectionTree::Selection UnclaimedTokens = SelectionTree::Unselected;
+ V.Claimed.claim(Begin, End, UnclaimedTokens);
+ if (UnclaimedTokens || V.Nodes.size() == 1) {
StringRef FileContent = AST.getSourceManager().getBufferData(File);
// Don't require the trailing newlines to be selected.
bool SelectedAll = Begin == 0 && End >= FileContent.rtrim().size();
@@ -333,10 +339,11 @@ private:
Nodes.emplace_back();
Nodes.back().ASTNode = std::move(Node);
Nodes.back().Parent = Stack.top();
- // Early hit detection never selects the whole node.
Stack.push(&Nodes.back());
- Nodes.back().Selected =
- claimRange(Early) ? SelectionTree::Partial : SelectionTree::Unselected;
+ claimRange(Early, Nodes.back().Selected);
+ // Early hit detection never selects the whole node.
+ if (Nodes.back().Selected)
+ Nodes.back().Selected = SelectionTree::Partial;
}
// Pops a node off the ancestor stack, and finalizes it. Pairs with push().
@@ -344,8 +351,7 @@ private:
void pop() {
Node &N = *Stack.top();
dlog("{1}pop: {0}", printNodeToString(N.ASTNode, PrintPolicy), indent(-1));
- if (auto Sel = claimRange(N.ASTNode.getSourceRange()))
- N.Selected = Sel;
+ claimRange(N.ASTNode.getSourceRange(), N.Selected);
if (N.Selected || !N.Children.empty()) {
// Attach to the tree.
N.Parent->Children.push_back(&N);
@@ -375,9 +381,10 @@ private:
// Perform hit-testing of a complete Node against the selection.
// This runs for every node in the AST, and must be fast in common cases.
// This is usually called from pop(), so we can take children into account.
- SelectionTree::Selection claimRange(SourceRange S) {
+ // The existing state of Result is relevant (early/late claims can interact).
+ void claimRange(SourceRange S, SelectionTree::Selection &Result) {
if (!S.isValid())
- return SelectionTree::Unselected;
+ return;
// toHalfOpenFileRange() allows selection of constructs in macro args. e.g:
// #define LOOP_FOREVER(Body) for(;;) { Body }
// void IncrementLots(int &x) {
@@ -391,17 +398,16 @@ private:
auto E = SM.getDecomposedLoc(Range->getEnd());
// Otherwise, nodes in macro expansions can't be selected.
if (B.first != SelFile || E.first != SelFile)
- return SelectionTree::Unselected;
+ return;
// Attempt to claim the remaining range. If there's nothing to claim, only
// children were selected.
- SelectionTree::Selection Result = Claimed.claim(B.second, E.second);
+ Claimed.claim(B.second, E.second, Result);
if (Result)
dlog("{1}hit selection: {0}",
SourceRange(SM.getComposedLoc(B.first, B.second),
SM.getComposedLoc(E.first, E.second))
.printToString(SM),
indent());
- return Result;
}
std::string indent(int Offset = 0) {
Modified: clang-tools-extra/trunk/clangd/refactor/tweaks/ExtractFunction.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/refactor/tweaks/ExtractFunction.cpp?rev=373442&r1=373441&r2=373442&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/refactor/tweaks/ExtractFunction.cpp (original)
+++ clang-tools-extra/trunk/clangd/refactor/tweaks/ExtractFunction.cpp Wed Oct 2 03:01:53 2019
@@ -113,15 +113,12 @@ const Node *getParentOfRootStmts(const N
return 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;
case SelectionTree::Selection::Partial:
- // Treat Partially selected VarDecl as completely selected since
- // SelectionTree doesn't always select VarDecls correctly.
- // FIXME: Remove this after D66872 is upstream)
- if (!CommonAnc->ASTNode.get<VarDecl>())
- return nullptr;
- LLVM_FALLTHROUGH;
+ // 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.
Modified: clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp?rev=373442&r1=373441&r2=373442&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp (original)
+++ clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp Wed Oct 2 03:01:53 2019
@@ -354,6 +354,8 @@ TEST(SelectionTest, Selected) {
#define ECHO(X) X
ECHO(EC^HO([[$C[[int]]) EC^HO(a]]));
]])cpp",
+ R"cpp( $C[[^$C[[int]] a^]]; )cpp",
+ R"cpp( $C[[^$C[[int]] a = $C[[5]]^]]; )cpp",
};
for (const char *C : Cases) {
Annotations Test(C);
Modified: clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp?rev=373442&r1=373441&r2=373442&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp (original)
+++ clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp Wed Oct 2 03:01:53 2019
@@ -540,13 +540,12 @@ TEST_F(ExtractFunctionTest, FunctionTest
EXPECT_EQ(apply("int x = 0; [[x++;]]"), "unavailable");
// We don't support extraction from lambdas.
EXPECT_EQ(apply("auto lam = [](){ [[int x;]] }; "), "unavailable");
+ // Partial statements aren't extracted.
+ EXPECT_THAT(apply("int [[x = 0]];"), "unavailable");
// Ensure that end of Zone and Beginning of PostZone being adjacent doesn't
// lead to break being included in the extraction zone.
EXPECT_THAT(apply("for(;;) { [[int x;]]break; }"), HasSubstr("extracted"));
- // FIXME: This should be unavailable since partially selected but
- // selectionTree doesn't always work correctly for VarDecls.
- EXPECT_THAT(apply("int [[x = 0]];"), HasSubstr("extracted"));
// FIXME: ExtractFunction should be unavailable inside loop construct
// initalizer/condition.
EXPECT_THAT(apply(" for([[int i = 0;]];);"), HasSubstr("extracted"));
@@ -554,7 +553,6 @@ TEST_F(ExtractFunctionTest, FunctionTest
EXPECT_THAT(apply(" [[int a = 5;]] a++; "), StartsWith("fail"));
// Don't extract return
EXPECT_THAT(apply(" if(true) [[return;]] "), StartsWith("fail"));
-
}
TEST_F(ExtractFunctionTest, FileTest) {
More information about the cfe-commits
mailing list