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