ilya-biryukov added inline comments.
================ Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:766 + [File, Code, Params, Reply = std::move(Reply), + this](llvm::Expected<FileEdits> Edits) mutable { + if (!Edits) ---------------- NIT: is capture of `this` redundant here? I could be missing something, though. ================ Comment at: clang-tools-extra/clangd/ClangdServer.cpp:334 + SourceLocation Loc = getBeginningOfIdentifier( Pos, AST.getSourceManager(), AST.getASTContext().getLangOpts()); ---------------- Why not do this before running rename? This would allow to: 1. return result without doing the actual (expensive) rename if there's no identifier under the cursor 2. reduce nesting of the (now large) large piece of code that runs rename: ``` auto Loc = getBeginningOfIdentifier(...); auto Range = ...; if (!Range) return llvm::None; if (!CrossFileRename) return *Range; // FIXME: assume cross-file rename always succeeds // do the rename... ``` ================ Comment at: clang-tools-extra/clangd/ClangdServer.cpp:353 return CB(InpAST.takeError()); - auto Changes = renameWithinFile(InpAST->AST, File, Pos, NewName, Index); - if (!Changes) - return CB(Changes.takeError()); + RenameInputs RInputs; + RInputs.MainFileCode = InpAST->Inputs.Contents; ---------------- Could we get a helper function (or constructor) to keep the call sites as simple as they used to be? `RenameInputs{Contents, File, NewName, Pos,...}` should also work just fine and I don't think it hurts readability, since all the parameters here are named the same as the fields and most of them have different types. ================ Comment at: clang-tools-extra/clangd/ClangdServer.cpp:360 + RInputs.Index = Index; + RInputs.VFS = FSProvider.getFileSystem(); + RInputs.GetDirtyBuffer = GetDirtyBuffer; ---------------- This can always be obtained from the AST, right? Why do we need it separately? ================ Comment at: clang-tools-extra/clangd/ClangdServer.cpp:370 + for (auto &E : *Edits) { + if (auto Err = reformatEdit(E.getValue(), Style)) + elog("Failed to format replacements: {0}", std::move(Err)); ---------------- Maybe use `llvm::joinErrors` to combine multiple failures into a single error? Should simplify the code. ================ Comment at: clang-tools-extra/clangd/SourceCode.h:226 }; +using FileEdits = llvm::StringMap<Edit>; ---------------- We should document this typedef, otherwise it does not provide much value. A doc comment from `ApplyEdits` should fit just nicely here. ================ Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:71 + bool CrossFile) { + // Filter out symbols that are unsupported in both rename mode. if (llvm::isa<NamespaceDecl>(&RenameDecl)) ---------------- NIT: in both rename **modes** ================ Comment at: clang-tools-extra/clangd/refactor/Rename.h:23 +using DirtyBufferGetter = + std::function<llvm::Optional<std::string>(PathRef Path)>; ---------------- ilya-biryukov wrote: > Could you document what does it mean for this function to return `llvm::None`? > I assume we read the contents from disk instead. > > Also worth documenting what should be returned for `MainFilePath`? > `llvm::None`? same value as `MainFileCode`? What does it return for `MainFilePath`? `llvm::None`? `MainFileCode`? ================ Comment at: clang-tools-extra/clangd/refactor/Rename.h:33 + llvm::StringRef MainFilePath; + llvm::StringRef MainFileCode; + ---------------- `MainFileCode` can be obtained from the AST. Why not do it? ================ Comment at: clang-tools-extra/clangd/refactor/Rename.h:40 + // When set, used by the rename to get file content for all rename-related + // files (except the main file). + // If there is no corresponding dirty buffer, we will use the file content ---------------- Why is `MainFile` special? Can it also be handled with the GetDirtyBuffer? ================ Comment at: clang-tools-extra/clangd/refactor/Rename.h:49 +/// in another file (per the index). +llvm::Expected<FileEdits> rename(ParsedAST &AST, const RenameInputs &RInputs); ---------------- Why isn't `AST` part of the `RenameInputs`? 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