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