[PATCH] D95231: [clangd] Selection handles CXXBaseSpecifier

[Nathan James via Phabricator via cfe-commits]

njames93 marked 2 inline comments as done.
njames93 added inline comments.


================
Comment at: clang-tools-extra/clangd/Selection.cpp:496
   }
+  bool TraverseCXXBaseSpecifier(const CXXBaseSpecifier &X) {
+    auto N = DynTypedNode::create(X);
----------------
sammccall wrote:
> can we not use traverseNode here, rather than repeating?
Yeah, especially now I'm changing it to actually traverse :)


================
Comment at: clang-tools-extra/clangd/unittests/SelectionTests.cpp:272-283
+          R"cpp(
+            struct Foo {};
+            struct Bar : [[private Fo^o]] {};
+          )cpp",
+          "CXXBaseSpecifier",
+      },
+      {
----------------
sammccall wrote:
> 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)
OK, funnily enough that change to find target was to fix renaming when the recordtypeloc was removed from the selection. That change will be redundant if i fix that, but do you still want it in here? 


================
Comment at: clang/lib/AST/ASTTypeTraits.cpp:196
     return SourceRange(C->getBeginLoc(), C->getEndLoc());
+  if (const auto *CBS = get<CXXBaseSpecifier>())
+    return CBS->getSourceRange();
----------------
sammccall wrote:
> nice, thanks!
> We should probably have a test for this (in ASTTypeTraitsTest.cpp there's a simple pattern to follow for sourcerange)
Not really trivial as there is no matcher for `cxxBaseSpecifier`. Annoyingly its my fault there isn't one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95231