sammccall created this revision.
sammccall added a reviewer: hokein.
Herald added subscribers: usaxena95, kadircet, arphaman.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.
Because declarators nest inside-out, we logically need to claim tokens for
parent declarators logically before child ones.
This is the ultimate reason we had problems with DeclaratorDecl, ArrayType etc.
However actually changing the order of traversal is hard, especially for nodes
that have both declarator and non-declarator children.
Since there's only a few TypeLocs corresponding to declarators, we just
have them claim the exact tokens rather than rely on nesting.
This fixes handling of complex declarators, like
`int (*Fun(OuterT^ype))(InnerType);`.
This avoids the need for the DeclaratorDecl early-claim hack, which is
removed.
Unfortunately the DeclaratorDecl early-claims were covering up an AST
anomaly around CXXConstructExpr, so we need to fix that up too.
Based on D116623 <https://reviews.llvm.org/D116623> and D116618
<https://reviews.llvm.org/D116618>
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D116630
Files:
clang-tools-extra/clangd/Selection.cpp
clang-tools-extra/clangd/unittests/SelectionTests.cpp
Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -204,9 +204,9 @@
{
R"cpp(
struct S { S(const char*); };
- S [[s ^= "foo"]];
+ [[S s ^= "foo"]];
)cpp",
- "CXXConstructExpr",
+ "VarDecl",
},
{
R"cpp(
@@ -231,7 +231,7 @@
R"cpp(
[[void (^*S)(int)]] = nullptr;
)cpp",
- "FunctionProtoTypeLoc",
+ "PointerTypeLoc",
},
{
R"cpp(
@@ -243,7 +243,7 @@
R"cpp(
[[void ^(*S)(int)]] = nullptr;
)cpp",
- "FunctionProtoTypeLoc",
+ "ParenTypeLoc",
},
{
R"cpp(
@@ -326,6 +326,8 @@
{"const int x = 1, y = 2; [[i^nt]] array[x][10][y];", "BuiltinTypeLoc"},
{"void func(int x) { int v_array[^[[x]]][10]; }", "DeclRefExpr"},
+ {"int (*getFunc([[do^uble]]))(int);", "BuiltinTypeLoc"},
+
// FIXME: the AST has no location info for qualifiers.
{"const [[a^uto]] x = 42;", "AutoTypeLoc"},
{"[[co^nst auto x = 42]];", "VarDecl"},
Index: clang-tools-extra/clangd/Selection.cpp
===================================================================
--- clang-tools-extra/clangd/Selection.cpp
+++ clang-tools-extra/clangd/Selection.cpp
@@ -639,11 +639,10 @@
// Nodes *usually* nest nicely: a child's getSourceRange() lies within the
// parent's, and a node (transitively) owns all tokens in its range.
//
- // Exception 1: child range claims tokens that should be owned by the parent.
- // e.g. in `void foo(int);`, the FunctionTypeLoc should own
- // `void (int)` but the parent FunctionDecl should own `foo`.
- // To handle this case, certain nodes claim small token ranges *before*
- // their children are traversed. (see earlySourceRange).
+ // Exception 1: when declarators nest, *inner* declarator is the *outer* type.
+ // e.g. void foo[5](int) is an array of functions.
+ // To handle this case, declarators are careful to only claim the tokens they
+ // own, rather than claim a range and rely on claim ordering.
//
// Exception 2: siblings both claim the same node.
// e.g. `int x, y;` produces two sibling VarDecls.
@@ -690,16 +689,13 @@
}
// Pushes a node onto the ancestor stack. Pairs with pop().
- // Performs early hit detection for some nodes (on the earlySourceRange).
void push(DynTypedNode Node) {
- SourceRange Early = earlySourceRange(Node);
dlog("{1}push: {0}", printNodeToString(Node, PrintPolicy), indent());
Nodes.emplace_back();
Nodes.back().ASTNode = std::move(Node);
Nodes.back().Parent = Stack.top();
Nodes.back().Selected = NoTokens;
Stack.push(&Nodes.back());
- claimRange(Early, Nodes.back().Selected);
}
// Pops a node off the ancestor stack, and finalizes it. Pairs with push().
@@ -721,52 +717,46 @@
Stack.pop();
}
- // Returns the range of tokens that this node will claim directly, and
- // is not available to the node's children.
- // Usually empty, but sometimes children cover tokens but shouldn't own them.
- SourceRange earlySourceRange(const DynTypedNode &N) {
- if (const Decl *D = N.get<Decl>()) {
- // We want constructor name to be claimed by TypeLoc not the constructor
- // itself. Similar for deduction guides, we rather want to select the
- // underlying TypeLoc.
- // FIXME: Unfortunately this doesn't work, even though RecursiveASTVisitor
- // traverses the underlying TypeLoc inside DeclarationName, it is null for
- // constructors.
- if (isa<CXXConstructorDecl>(D) || isa<CXXDeductionGuideDecl>(D))
- return SourceRange();
- // This will capture Field, Function, MSProperty, NonTypeTemplateParm and
- // VarDecls. We want the name in the declarator to be claimed by the decl
- // and not by any children. For example:
- // void [[foo]]();
- // int (*[[s]])();
- // struct X { int [[hash]] [32]; [[operator]] int();}
- if (const auto *DD = llvm::dyn_cast<DeclaratorDecl>(D))
- return DD->getLocation();
- } else if (const auto *CCI = N.get<CXXCtorInitializer>()) {
- // : [[b_]](42)
- return CCI->getMemberLocation();
- }
- return SourceRange();
- }
-
// Claim tokens for N, after processing its children.
// By default this claims all unclaimed tokens in getSourceRange().
// We override this if we want to claim fewer tokens (e.g. there are gaps).
void claimTokensFor(const DynTypedNode &N, SelectionTree::Selection &Result) {
+ // CXXConstructExpr often shows implicit construction, like `string s;`.
+ // Don't associate any tokens with it unless there's some syntax like {}.
+ // This prevents it from claiming 's', its primary location.
+ if (const auto *CCE = N.get<CXXConstructExpr>()) {
+ claimRange(CCE->getParenOrBraceRange(), Result);
+ return;
+ }
+ // ExprWithCleanups is always implicit. It often wraps CXXConstructExpr.
+ // Prevent it claiming 's' in the case above.
+ if (N.get<ExprWithCleanups>())
+ return;
+
+ // Declarators nest "inside out", with parent types inside child ones.
+ // Instead of claiming the whole range (clobbering parent tokens), carefully
+ // claim the tokens owned by this node and non-declarator children.
+ // (We could manipulate traversal order instead, but this is easier).
if (const auto *TL = N.get<TypeLoc>()) {
- // e.g. EltType Foo[OuterSize][InnerSize];
- // ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ArrayTypeLoc (Outer)
- // ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |-ArrayTypeLoc (Inner)
- // ~~~~~~~ | |-RecordType
- // ~~~~~~~~~ | `-Expr (InnerSize)
- // ~~~~~~~~~ `-Expr (OuterSize)
- // Inner ATL must not claim its whole SourceRange, or it clobbers Outer.
- if (TL->getAs<ArrayTypeLoc>()) {
- claimRange(TL->getLocalSourceRange(), Result);
+ if (auto PTL = TL->getAs<ParenTypeLoc>()) {
+ claimRange(PTL.getLParenLoc(), Result);
+ claimRange(PTL.getRParenLoc(), Result);
+ return;
+ }
+ if (auto ATL = TL->getAs<ArrayTypeLoc>()) {
+ claimRange(ATL.getBracketsRange(), Result);
+ return;
+ }
+ if (auto PTL = TL->getAs<PointerTypeLoc>()) {
+ claimRange(PTL.getStarLoc(), Result);
+ return;
+ }
+ if (auto FTL = TL->getAs<FunctionTypeLoc>()) {
+ claimRange(
+ SourceRange(FTL.getParensRange().getBegin(), FTL.getEndLoc()),
+ Result);
return;
}
- // FIXME: maybe LocalSourceRange is a better default for TypeLocs.
- // It doesn't seem to be usable for FunctionTypeLocs.
}
claimRange(getSourceRange(N), Result);
}
@@ -774,7 +764,7 @@
// 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.
- // The existing state of Result is relevant (early/late claims can interact).
+ // The existing state of Result is relevant.
void claimRange(SourceRange S, SelectionTree::Selection &Result) {
for (const auto &ClaimedRange :
UnclaimedExpandedTokens.erase(TokenBuf.expandedTokens(S)))
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
