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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

LG, just some style nits, thanks!



================
Comment at: clang-tools-extra/clangd/XRefs.cpp:226
 
-  // Emit all symbol locations (declaration or definition) from AST.
-  for (const Decl *D : getDeclAtPosition(AST, SourceLocationBeg)) {
-    auto Loc =
-        makeLocation(AST.getASTContext(), spellingLocIfSpelled(findName(D), SM),
-                     *MainFilePath);
-    if (!Loc)
-      continue;
-
-    Result.emplace_back();
-    if (auto *ND = dyn_cast<NamedDecl>(D))
-      Result.back().Name = printName(AST.getASTContext(), *ND);
-    Result.back().PreferredDeclaration = *Loc;
-    // DeclInfo.D is always a definition if possible, so this check works.
-    if (getDefinition(D) == D)
-      Result.back().Definition = *Loc;
-
-    // Record SymbolID for index lookup later.
-    if (auto ID = getSymbolID(D))
-      ResultIndex[*ID] = Result.size() - 1;
+  // For getDeclAtPosition(), use the actual input location, not the location
+  // of the beginning of the identifier, because SelectionTree may select
----------------
rather than justifying *not* doing something here, can we make it clearer that macros are the special case?
maybe move the "SourceLocationBeg" initialization under the "macros are simple" comment, and rename it to "MaybeMacroLocation" or so.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:233
+  } else {
+    log("locateSymbolAt: {0}", L.takeError());
+  }
----------------
log should have more context, and be an elog instead.
Return afterward?


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:236
+
+  // If we found a macro, don't bother calling getDeclAtPosition(). It would
+  // just return declarations referenced from the macro's expansion.
----------------
I don't really understand this comment.

If the intention is just not to look at the AST or index when we have a macro (which seems reasonable to me), then just `return Result` instead of `HaveMacro = true` above?


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:896
   // TODO: should we handle macros, too?
-  auto Decls = getDeclAtPosition(AST, Loc);
+  // We also want the targets of using-decls, so we include
+  // DeclRelation::Underlying.
----------------
want -> show references to


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1133
+  DeclRelationSet Relations =
+      DeclRelation::TemplatePattern | DeclRelation::Alias;
+  auto Decls = getDeclAtPosition(AST, SourceLocationBeg, Relations);
----------------
Are you sure you don't want type hierarchy to work on aliases to record types? (i.e. specify underlying rather than alias)


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:526
+
+  // Test constructor targeting. This currently works if the cursor
+  // is just before the opening parenthesis.
----------------
I'd suggest dropping these two lines of comment. The fact that it's testing constructors is clear from the range name. The FIXME comment describes the future change.


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:528
+  // is just before the opening parenthesis.
+  // FIXME: Ideally, we would like this to work for the whole
+  // identifier, and in cases where there is no parenthesis (e.g. case #7).
----------------
This could just be `// FIXME: target constructor instead of class.`. 
We already have a test, so the comment belongs above point("9") instead of here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69237