[PATCH] D67826: [clangd] A helper to find explicit references and their names

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

Thanks this mostly LG, but I think we need some tests feel free to copy the ones in D66647 <https://reviews.llvm.org/D66647> as a start point. But I believe coverage for this one is bigger, as this tries to figure-out all references, whereas the define-inline patch only cares about references to non-local decls in contexts that can have a nested name specifier.



================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:369
+llvm::SmallVector<const NamedDecl *, 1>
+explicitReferenceTargets(ast_type_traits::DynTypedNode N,
+                         DeclRelationSet Mask = {}) {
----------------
could you add some comments on what it does


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:373
+                   DeclRelation::TemplateInstantiation)) &&
+         "namedTarget handles templates on its own");
+  auto Decls = allTargetDecls(N);
----------------
what is `namedTarget` ?


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:376
+
+  // We prefer to return template instantiation, but fallback to template
+  // pattern if instantiation is not available.
----------------
can we simplify this by populating two vectors, Instantiations and Patterns, and then returning the patterns iff instantiations are empty?


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:381
+      llvm::find_if(Decls, [&](std::pair<const Decl *, DeclRelationSet> D) {
+        if (D.second & ~Mask)
+          return false;
----------------
why do we discard aliases(specifically underlying decls) ? for example if we have `vector<int>` I believe decl(`template<> std::vector<int>`) will be marked with both TemplateInst and Underlying right ?


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:391
+  for (auto &D : Decls) {
+    if (D.second & ~Mask)
+      continue;
----------------
same here for ignoring underlying decls


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:399
+Optional<ReferenceLoc> refInDecl(const Decl *D) {
+  struct Visitor : ConstDeclVisitor<Visitor> {
+    llvm::Optional<ReferenceLoc> Ref;
----------------
these three(usingdirective,using,namespacealias) decls are the only ones that can have a nestednamespecifier.

Are we sure these are the only ones that can reference a decl? For example what about a `ConstructorDecl` with initializers referencing `FieldDecl`s, I believe this function should rather return multiple locations in such cases.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:409
+    void VisitUsingDecl(const UsingDecl *D) {
+      Ref = ReferenceLoc{D->getQualifierLoc(), D->getUsingLoc(),
+                         explicitReferenceTargets(DynTypedNode::create(*D),
----------------
I think `getUsingLoc` will return the location for `using` keyword, not the name location. Maybe use `getLocation` ?


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:426
+
+Optional<ReferenceLoc> refInExpr(const Expr *E) {
+  struct Visitor : ConstStmtVisitor<Visitor> {
----------------
again I believe this list is non-exhaustive, e.g. designatedinitilaizers. and again this also needs to return a vector instead.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:432
+      Ref = ReferenceLoc{
+          E->getQualifierLoc(), E->getNameInfo().getLoc(), {E->getDecl()}};
+    }
----------------
I believe it is better to return `getFoundDecl` then `getDecl`, the former respects using declarations.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:506
+
+llvm::Optional<ReferenceLoc>
+explicitReference(ast_type_traits::DynTypedNode N) {
----------------
can we move this below `ExplicitReferenceCollector` to prevent splitting anon namespace?


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:548
+  bool TraverseElaboratedTypeLoc(ElaboratedTypeLoc L) {
+    // ElaboratedTypeLoc will reports information for its inner type loc.
+    // Otherwise we loose information about inner types loc's qualifier.
----------------
why not just traversenestednamespecifier and `visitNode(L)` instead of calling the base::traverse ?


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:570
+    visitNode(DynTypedNode::create(L));
+    // Inner type is missing information about its qualifier, skip it.
+    if (auto TL = L.getTypeLoc())
----------------
why not just visitNode(L) and traverse(prefix(L)) instead of calling base?


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:581
+      return;
+    // FIXME: this should be done by nodeReference.
+    if (Ref->NameLoc.isInvalid() || Ref->NameLoc.isMacroID())
----------------
what is `nodeReference`?


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:594
+
+void findExplicitReferences(Stmt *S,
+                            llvm::function_ref<void(ReferenceLoc)> Out) {
----------------
So this endpoint doesn't seem to have downsides above(not visiting all referenced decls) as it performs traversal through an extra ASTVisitor.

What is the point of exposing the first method? Maybe we can getaway with not exposing it or strictly mentioning it might not return references in the children nodes?


================
Comment at: clang-tools-extra/clangd/FindTarget.h:103
+/// (!) For the purposes of this function declarations are not considered to be
+///     references. However, declarations can have:wa references inside them,
+///     e.g. 'namespace foo = std' references namespace 'std' and this function
----------------
drop `:wa` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67826