ilya-biryukov added inline comments.
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:342
+ // A snapshot of all file dirty buffers.
+ llvm::StringMap<std::string> SnapShot = WorkScheduler.getAllFileContents();
auto Action = [File = File.str(), NewName = NewName.str(), Pos, WantFormat,
----------------
NIT: s/SnapShot/Snapshot
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:371
+ }
+ CB(std::move(*Edits));
};
----------------
NIT: `return CB(std::move(*Edits));`
to keep all calls to `CB` consistent.
================
Comment at: clang-tools-extra/clangd/TUScheduler.h:186
+
+ // std::vector<std::>
/// Schedule an async task with no dependencies.
----------------
NIT: seems like a leftover from the previous version. Maybe remove?
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:48
+ auto ID = getSymbolID(&D);
+ assert(ID);
+ Req.IDs.insert(*ID);
----------------
NIT: this assert is redundant, `Optional` asserts it has a value when `operator*` is called.
The code could be simplified to the equivalent: `Refs.insert(*getSymbolID(&D))`
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:135
+ // Note: within-file rename does support this through the AST.
+ if (const auto *S = llvm::dyn_cast<CXXMethodDecl>(&RenameDecl))
+ if (S->isVirtual())
----------------
nit: add braces to the outer if
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:244
+ return EndOffset.takeError();
+ return std::make_pair<>(*StartOffset, *EndOffset);
+};
----------------
NIT: remove `<>`. should still work, right?
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:297
+ FileEdits Results;
+ std::string OldName = RenameDecl->getNameAsString();
+ for (const auto &FileAndOccurrences : AffectedFiles) {
----------------
We don't seem to use it. Remove? Or am I missing the usage?
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:345
+ SourceLocation SourceLocationBeg =
+ SM.getMacroArgExpandedLocation(getBeginningOfIdentifier(
+ RInputs.Pos, SM, AST.getASTContext().getLangOpts()));
----------------
Why is this different from `prepareRename`, which does not call `getMacroArgExpandedLocation`?
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:379
+ return FileEdits(
+ {std::make_pair<>(RInputs.MainFilePath,
+ Edit{MainFileCode, std::move(*MainFileRenameEdit)})});
----------------
NIT: remove `<>`
================
Comment at: clang-tools-extra/clangd/refactor/Tweak.h:80
llvm::Optional<std::string> ShowMessage;
/// A mapping from file path(the one used for accessing the underlying VFS)
/// to edits.
----------------
The comment is now redundant, since the typedef has the same comment.
================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:27
+
+std::unique_ptr<RefSlab> buildRefSlab(const Annotations &Code,
+ llvm::StringRef SymbolName,
----------------
Could you document what this function is doing?
================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:52
+ std::pair</*InitialCode*/ std::string, /*CodeAfterRename*/ std::string>>
+applyRename(FileEdits FE) {
+ std::vector<std::pair<std::string, std::string>> Results;
----------------
NIT: I suggest to call it `applyEdits`, there's nothing rename-specific in this function.
================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:115
ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError();
- auto ApplyResult = llvm::cantFail(
- tooling::applyAllReplacements(Code.code(), *RenameResult));
- EXPECT_EQ(expectedResult(Code, NewName), ApplyResult);
+ EXPECT_THAT(applyRename(std::move(*RenameResult)),
+ UnorderedElementsAre(
----------------
NIT: maybe simplify to `EXPECT_EQ(applyRename(...).second, expectedResult(Code, NewName))`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69263/new/
https://reviews.llvm.org/D69263