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

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Thanks for putting this together, this is really neat and will indeed complement D72874 <https://reviews.llvm.org/D72874> nicely!

I have one high-level comment about the interface, the rest are nits.



================
Comment at: clang-tools-extra/clangd/XRefs.cpp:337
+  const SourceManager &SM = TB.sourceManager();
+  auto Pos = SM.getDecomposedLoc(SpellingLoc);
+  llvm::StringRef Code = SM.getBufferData(Pos.first);
----------------
nit: I think separate names for the `FileID` and the position would read better


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:369
+  auto Consider = [&](const syntax::Token &Tok) {
+    if(!(Tok.kind() == tok::identifier && Tok.text(SM) == Word))
+      return false;
----------------
nit: space after `if`


================
Comment at: clang-tools-extra/clangd/XRefs.h:56
 
+// If SpellingLoc points at a "word" that does not correspond to an expanded
+// token (e.g. in a comment, a string, or a PP disabled region), then try to
----------------
Do we want to bake this condition into the interface of this function, or would it be more appropriate to just tell the caller whether it's a real identifier token or not?

In particular, given our [plan for handling some dependent cases](https://reviews.llvm.org/D72874#1901722), the caller may want to do something along the lines of `if (notRealIdentifier || isDependent) { /* use the textual heuristic */ }`.


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:871
+TEST(LocateSymbol, NearbyIdentifier) {
+  const char *Tests[] = {
+    R"cpp(
----------------
Tangentially related, but what do you think about the issue I raised in [this mailing list thread](https://lists.llvm.org/pipermail/clangd-dev/2019-November/000579.html) about testcase style?


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:903
+    R"cpp(
+      // prefer nearest occurrence
+      int hello;
----------------
// (taking into account the penalty for going backwards)


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:930
+    llvm::Optional<Range> Nearby;
+    if (const auto*Tok = findNearbyIdentifier(
+        cantFail(sourceLocationInMainFile(SM, T.point())), AST.getTokens()))
----------------
nit: space after `*`


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:937
+    else
+      EXPECT_THAT(Nearby, T.range()) << Test;
+  }
----------------
`EXPECT_EQ`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75479