nridge marked an inline comment as done.
nridge added inline comments.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:139
+// that constructor. FIXME(nridge): this should probably be handled in
+// targetDecl() itself.
+const CXXConstructorDecl *findCalledConstructor(const SelectionTree::Node *N) {
----------------
ilya-biryukov wrote:
> nridge wrote:
> > ilya-biryukov wrote:
> > > sammccall wrote:
> > > > nridge wrote:
> > > > > I would appreciate some tips on how we could handle this in
> > > > > `targetDecl()`. Please see more specific comments below.
> > > > My thinking is basically:
> > > > - C++ syntax is vague and doesn't give us great ways of referring
> > > > directly to constructors.
> > > > - there's value to simplicity, and I've found go-to-definition
> > > > additionally finding implicit nodes to be confusing as often as useful.
> > > > I think on balance and given the constraints of LSP, we should consider
> > > > dropping this and having implicit references be returned by find-refs
> > > > but not targetable by go-to-def/hover/etc. It's OK for simplicity of
> > > > implementation to be a concern here.
> > > > - the node that targets the constructor is the CXXConstructExpr.
> > > > Treating the VarDecl conditionally as a reference to the constructor,
> > > > while clever, seems like a hack. Ideally the fix is to make
> > > > SelectionTree yield the CXXConstructExpr, not to change TargetDecl.
> > > > - CXXConstructExpr is only sometimes implicit. SelectionTree should
> > > > return it for the parens in `Foo()` or `Foo x()`. And ideally for the
> > > > equals in `Foo x = {}`, though I think there's no CXXConstructExpr in
> > > > the AST in that case :-(
> > > > - When the AST modelling is bad (as it seems to be for some aspects
> > > > CXXConstructExpr) we should consider accepting some glitches and trying
> > > > to improve things in clang if they're important. Hacking around them
> > > > will lead to inconsistencies between different clangd features.
> > > >
> > > > The TL;DR is I think I'd be happy to drop this special case and try to
> > > > make SelectionTree surface CXXConstructExpr more often, even if it
> > > > changes some behavior.
> > > +1 to the idea of landing this and changing behavior.
> > > I don't think we're loosing much in terms of functionality and I
> > > personally like the new code much more.
> > FWIW, I do find being able to get from a variable declaration to the
> > invoked constructor to be very useful.
> >
> > If the class has several constructors, there's no other easy way to
> > determine which one is being invoked (only other thing I can think of is to
> > perform "find references" on each constructor and see which one includes
> > the variable declaration as a reference, which is a lot more tedious), and
> > I think that's an important thing to be able to do (just like for named
> > functions).
> >
> > So I'd definitely like to keep this case working. However, I'd be fine with
> > only having the parens or braces target the constructor. (That's still
> > slightly annoying, as you have to place the cursor more precisely, and it
> > also may not be obvious to users, but at least there's a way to do it.) I'm
> > also fine with changing the behaviour for now, and deferring constructor
> > targeting to a follow-up, as there are other benefits this patch brings.
> We discussed this offline: finding constructor references is useful, but
> current approach does not generalize very well, e.g. for implicit constructor
> calls:
> ```
> struct Foo {
> Foo(int);
> };
>
> int func(Foo a, Foo b);
> int test() {
> func(1, 2); // no way to find Foo(1) and Foo(2) calls.
> }
> ```
>
> So we'd probably want some other mechanism to expose *all* possible
> references.
> In any case, changing behavior now and tweaking it with a follow-up patch is
> probably the best way to proceed with this change.
> So we'd probably want some other mechanism to expose *all* possible
> references.
Any ideas for what such a mechanism might look like?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69237/new/
https://reviews.llvm.org/D69237
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
