ilya-biryukov 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) {
----------------
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.
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
