ilya-biryukov marked an inline comment as done.
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:
> > 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?
We were discussing this offline, a few options that came up are:
Expose tree view of the AST (that links to the source code) with implicit
nodes, allow to navigate from the conversion nodes to the references.
Code lens in VSCode, i.e. in-editor annotations right in the middle of the code
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
