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