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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
