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