[PATCH] D66751: [clangd] Add targetDecl(), which determines what declaration an AST node refers to.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Mostly LGTM, thanks! Just a few clarifying questions and suggestions

This API allows us to get a target declaration for a reference, but does not help with finding the source locations for those references. Do you plan to put this into the API? Have this as a separate function?
E.g. finding a source location of `field` for `DeclRefExpr` produced for `MyBase::field` seems to be the same amount of work (in general case) as finding the `Decl*` of `field`.



================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:374
+  const char *Sep = "";
+  for (unsigned I = 0; I < RS.S.size(); ++I)
+    if (RS.S.test(I)) {
----------------
NIT: maybe add braces around multi-line body of the for statement?


================
Comment at: clang-tools-extra/clangd/FindTarget.h:70
+llvm::SmallVector<const Decl *, 1>
+targetDecl(const ast_type_traits::DynTypedNode &, DeclRelationSet Mask);
+
----------------
Could we add convenience overloads for `Expr*`, `QualType`, `TypeLoc` and maybe other interesting cases?
Having to call `DynTypedNode::create` for each invocation of these two functions would feel awkward, saving some typing for the clients seems to be worth a little boilerplate.


================
Comment at: clang-tools-extra/clangd/FindTarget.h:86
+  /// This declaration is an alias that was referred to.
+  Alias,
+  /// This is the underlying declaration for an alias, decltype etc.
----------------
Could you provide examples for `Alias` and `Underlying` in the comments?
One with a template and one with a namespace alias


================
Comment at: clang-tools-extra/clangd/FindTarget.h:92
+  /// (e.g. a delegating constructor has a pure-lexical reference to the class)
+  PureLexical,
+};
----------------
Could you also provide an example here?
It this a delegating constructor in the constructor-init-list?


================
Comment at: clang-tools-extra/clangd/FindTarget.h:103
+  DeclRelationSet() = default;
+  DeclRelationSet(DeclRelation R) { S.set(static_cast<unsigned long long>(R)); }
+
----------------
Why not `unsigned`? What are the interesting corner cases?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66751/new/

https://reviews.llvm.org/D66751