sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

(I don't see a new snapshot but LG with the traverseNode change)



================
Comment at: clang-tools-extra/clangd/unittests/SelectionTests.cpp:272-283
+          R"cpp(
+            struct Foo {};
+            struct Bar : [[private Fo^o]] {};
+          )cpp",
+          "CXXBaseSpecifier",
+      },
+      {
----------------
njames93 wrote:
> 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? 
Up to you, but I'd lean towards keeping it and adding a FindTarget test for 
`struct S : ^private T` or so, which is the visible change.


================
Comment at: clang/lib/AST/ASTTypeTraits.cpp:196
     return SourceRange(C->getBeginLoc(), C->getEndLoc());
+  if (const auto *CBS = get<CXXBaseSpecifier>())
+    return CBS->getSourceRange();
----------------
njames93 wrote:
> 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.
Oh, I've been down this rabbithole recently... clangd support for attrs -> 
DynTypedNode support for Attr -> matchers for use in tests -> matchers get 
complicated...

Let's not block on this (and I need to revive my attr patch that got too big...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95231

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to