[PATCH] D69237: Refactor getDeclAtPosition() to use SelectionTree + targetDecl()

[Ilya Biryukov via Phabricator via cfe-commits]

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