[PATCH] D95231: [clangd] Selection handles CXXBaseSpecifier

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/Selection.cpp:496
   }
+  bool TraverseCXXBaseSpecifier(const CXXBaseSpecifier &X) {
+    auto N = DynTypedNode::create(X);
----------------
can we not use traverseNode here, rather than repeating?


================
Comment at: clang-tools-extra/clangd/unittests/SelectionTests.cpp:272-283
+          R"cpp(
+            struct Foo {};
+            struct Bar : [[private Fo^o]] {};
+          )cpp",
+          "CXXBaseSpecifier",
+      },
+      {
----------------
njames93 wrote:
> Is this good behaviour, or should Foo selection resolve as a RecordTypeLoc, That is the behaviour previously observed for these 2 cases. And instead have CXXBaseSpecifier as its parent selection?
Yes, this should be a typeloc with the CXXBaseSpecifier as its parent, unless there's a really compelling reason to make an exception.

Two main reasons:
 - it matches RecursiveASTVisitor's idea of the tree shape, which people mostly understand. If we diverge, it'll be a special case we need to remember and so potentially a source of bugs.
 - This allows code that handles types generically to work with fewer special cases.
  (I do like the FindTarget change though - it seems reasonable to allow go-to-def on e.g. the `public` keyword)
  It still preserves flexibility if we want to handle types as base-specifiers specially (we can examine the parent)


================
Comment at: clang/lib/AST/ASTTypeTraits.cpp:196
     return SourceRange(C->getBeginLoc(), C->getEndLoc());
+  if (const auto *CBS = get<CXXBaseSpecifier>())
+    return CBS->getSourceRange();
----------------
nice, thanks!
We should probably have a test for this (in ASTTypeTraitsTest.cpp there's a simple pattern to follow for sourcerange)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95231/new/

https://reviews.llvm.org/D95231