[PATCH] D75479: [clangd] go-to-def on names in comments etc that are used nearby.

[Nathan Ridge via Phabricator via cfe-commits]

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

Thanks, Sam, this looks great!



================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:883
 
+static bool isLikelyIdentifier(llvm::StringRef Word, StringRef Before,
+                               StringRef After) {
----------------
nit: qualify StringRef or not consistently


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:891
+    return true;
+  // Doxygen tags like \c foo indicate identifiers.
+  // Don't search too far back.
----------------
It's interesting to note that clang has a lexer and parser for doxygen comments (see e.g. `RawComment::parse()`), so we could conceivably do something more structured, but it's probably not worth the effort.


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:938
+          TB.expandedTokens(SM.getMacroArgExpandedLocation(T.location()));
+      if (!Expanded.empty() && Expanded.size() == 1 &&
+          Expanded.front().text(SM) == Result.Text)
----------------
`Expanded.size() == 1` implies `!Expanded.empty()`


================
Comment at: clang-tools-extra/clangd/SourceCode.h:244
+  // - Text is identifier-like (e.g. "foo_bar")
+  // - Text is surrounded by quotes (e.g. Foo in "// returns `Foo`")
+  bool LikelyIdentifier = false;
----------------
quotes --> backticks?


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:451
+    assert(SM.getFileID(Loc) == File && "spelled token in wrong file?");
+    unsigned Line = SM.getLineNumber(File, SM.getFileOffset(Loc));
+    if (Line > WordLine)
----------------
Is this any different from `getSpellingLineNumber(Loc)`?


================
Comment at: clang-tools-extra/clangd/unittests/SourceCodeTests.cpp:392
+  EXPECT_TRUE(word("// bar::[[f^oo]] ").LikelyIdentifier);
+  EXPECT_TRUE(word("// [[f^oo]]::bar ").LikelyIdentifier);
+}
----------------
Maybe test the initialism thing with `EXPECT_FALSE(word("// [[H^TTP]] ").LikelyIdentifier);`


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:1028
+      int [[hello]];
+      const char* greeting = "h^ello, world";
+    )cpp",
----------------
What's the rationale for supporting string literals for the nearby-ident heuristic, but not the index heuristic?


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:1032
+      R"cpp(
+      // can refer to macro invocations (even if they expand to nothing)
+      #define INT int
----------------
(Did you mean to write a test case where the macro invocation expands to nothing?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75479