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