[PATCH] D69934: [clangd] Implement rename by using SelectionTree and findExplicitReferences.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Thanks, LG. The only important is about testing the `foo.inc` case separately from the rest of rename tests.



================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:180
 
+const NamedDecl &getPrimaryTemplateOrThis(const NamedDecl &ND) {
+  if (const auto *T = ND.getDescribedTemplate())
----------------
NIT: inline this into the single caller.
Should simplify the code.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:258
+    // Filter out locations not from main file.
+    // We are safe for most cases as we only visit main file AST, but not
+    // always, locations could come from an #include file, e.g.
----------------
NIT: could probably shorten the whole sentence to `We traverse only main file decls, but locations could come from an #include file, e.g.`


================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:45
   llvm::StringRef Tests[] = {
-      // Rename function.
+      // rename function
       R"cpp(
----------------
NIT: Keep periods at the end


================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:371
     auto TU = TestTU::withCode(Code.code());
+    TU.AdditionalFiles["foo.inc"] = R"cpp(
+      #define Macro(X) X
----------------
Maybe test this separately? It's only used in one test.
Having this added for all tests cases is somewhat confusing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69934