[PATCH] D116623: [clangd][WIP] Remove the earlySourceRange hack in SelectionTree.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: sammccall.
Herald added subscribers: usaxena95, kadircet, arphaman.
hokein requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

This is an attempt to remove the existing earlySourceRange hack in favor
of the claimed range concept.

Not sure it is worth doing it, existing tests are passed except there is
one small regression about ConstructExpr.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116623

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(
Index: clang-tools-extra/clangd/Selection.cpp
===================================================================
--- clang-tools-extra/clangd/Selection.cpp
+++ clang-tools-extra/clangd/Selection.cpp
@@ -690,16 +690,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,34 +718,6 @@
     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).
@@ -776,6 +745,15 @@
         return;
       }
 
+      // getLocalSourceRange is the parens range for most FunctionProtoTypeLocs,
+      // but not true for a trailing-return-type function.
+      //
+      // auto foo() -> int {}
+      //         XX   <- range we want to claim
+      // XXXXXXXXXXX  <- getLocalSourceRange
+      if (auto A = TL->getAs<FunctionProtoTypeLoc>())
+        return claimRange(A.getParensRange(), Result);
+
       // We use LocalSourceRange as a default claimed range for TypeLocs -- it
       // is better than getSourceRange (which returns a range covering itself
       // and its children), because we want to claim fewer tokens (multiple
@@ -789,6 +767,11 @@
       // Inner ATL must not claim its whole SourceRange, or it clobbers Outer.
       return claimRange(TL->getLocalSourceRange(), Result);
     }
+    if (const auto *CCE = N.get<CXXConstructExpr>())
+      return claimRange(CCE->getParenOrBraceRange(), Result);
+    // Don't claim any tokens for ExprWithCleanups.
+    if (const auto *CCE = N.get<ExprWithCleanups>())
+      return;
     claimRange(getSourceRange(N), Result);
   }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D116623.397389.patch
Type: text/x-patch
Size: 4105 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20220104/80e24a34/attachment.bin>