hokein 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) ---------------- ilya-biryukov wrote: > NIT: is capture of `this` redundant here? > > I could be missing something, though. it is not redundant, we access the `this->DraftMgr` in the lambda body. ================ Comment at: clang-tools-extra/clangd/ClangdServer.cpp:334 + SourceLocation Loc = getBeginningOfIdentifier( Pos, AST.getSourceManager(), AST.getASTContext().getLangOpts()); ---------------- ilya-biryukov wrote: > 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... > ``` Good point. ================ Comment at: clang-tools-extra/clangd/ClangdServer.cpp:360 + RInputs.Index = Index; + RInputs.VFS = FSProvider.getFileSystem(); + RInputs.GetDirtyBuffer = GetDirtyBuffer; ---------------- ilya-biryukov wrote: > This can always be obtained from the AST, right? > Why do we need it separately? yes, obtained from the AST. ================ 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)); ---------------- ilya-biryukov wrote: > Maybe use `llvm::joinErrors` to combine multiple failures into a single error? > Should simplify the code. I don't see using `llvm::joinErrors` can simplify the code here, `joinErrors` accepts two `Error` objects, and there is no way to create an empty Error object, we may end up with more code like: ``` llvm::Optional<llvm::Error> Err; for (auto &E : *Edits) { if (auto E = reformatEdit(E.getValue(), Style)) { if (!Err) Err = std::move(E); else Err = joinErrors(std::move(*Err), std::move(Err)); } } ``` ================ Comment at: clang-tools-extra/clangd/refactor/Rename.h:33 + llvm::StringRef MainFilePath; + llvm::StringRef MainFileCode; + ---------------- ilya-biryukov wrote: > `MainFileCode` can be obtained from the AST. Why not do it? `ParsedAST` doesn't provide such API to get the file code, I assume you mean `InputsAndAST`? The `MainFileCode` is from `InputsAndAST`. ================ 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 ---------------- ilya-biryukov wrote: > Why is `MainFile` special? Can it also be handled with the GetDirtyBuffer? I believe we should use the content from `InputsAndAST`, which is corresponding to the main AST (the content from `GetDiryBuffer` may not be reflected to the AST). 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