[PATCH] D76451: [clangd] Enable textual fallback for go-to-definition on dependent names

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for your patience!



================
Comment at: clang-tools-extra/clangd/XRefs.cpp:154
+getDeclAtPosition(ParsedAST &AST, SourceLocation Pos, DeclRelationSet Relations,
+                  bool *IsDependentName = nullptr) {
   unsigned Offset = AST.getSourceManager().getDecomposedSpellingLoc(Pos).second;
----------------
shovelling this IsDependentName bool around seems a bit ad-hoc. The actual node doesn't live long enough, but can you make it an ASTNodeKind instead, and move the isDependentName check down to locateTextually where it makes sense to be that specific?


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:355
+                      bool IsDependent) {
+  // Don't use heuristics if this is a real non-dependent identifier, or not an
+  // identifier.
----------------
This sentence is hard to parse now, and also doesn't really justify the behaviour.
Suggest leaving it alone and adding:
// Exception: dependent names, because XYZ


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:569
       ASTResults = locateASTReferent(NearbyIdent->location(), NearbyIdent, AST,
-                                     *MainFilePath, Index);
+                                     *MainFilePath, Index, &IsDependentName);
       if (!ASTResults.empty())
----------------
the way the two (possible!) writes get combined is not obvious here.

I'd suggest using two different variables and writing `IsDependent || IsNearbyIdentDependent` in the usage below, if that's the intent.


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:694
+
+TEST(LocateSymbol, TextualDependent) {
+  const char *Tests[] = {
----------------
this has a loop but only one test...


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:698
         struct Foo {
-          void uniqueMethodName();
+          void [[uniqueMethodName]]();
         };
----------------
move these into the header so it can't be identifier-based heuristics? (And so the test doesn't change behaviour when we turn that on)


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:810
         struct Bar {
           void $BarLoc[[uniqueMethodName]]();
         };
----------------
this looks like a very close superset of TextualDependent, do we need both?

Again, these decls should move into the header I think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76451