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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits