njames93 marked 2 inline comments as done.
njames93 added inline comments.
================
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:
> > 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.
I think I should remove it, then add it back in a follow up. It was needed to
fix a issue in here that was then resolved in here.
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
