ilya-biryukov added inline comments.
================ 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)); ---------------- hokein wrote: > 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)); > } > } > ``` Works nicely if you start with `Error::success` ``` auto Err = Error::success(); for (...) { Err = llvm::joinErrors(reformatEdit(E.getValue(), Style)); } if (Err) return CB(std::move(Err)); ``` ================ Comment at: clang-tools-extra/clangd/ClangdServer.cpp:360 + GetDirtyBuffer}; + auto Edits = clangd::rename(RInputs); + if (!Edits) ---------------- NIT: inlining `RInputs` should save quite a few lines of code ================ Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:91 + if (!CrossFile) { + // Within-file rename. + auto &ASTCtx = RenameDecl.getASTContext(); ---------------- NIT: this comment is redundant, previous line says the same thing ================ Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:107 + if (!Index) + return NoIndexProvided; + ---------------- Why isn't this a scope enum in the first place? ================ Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:128 + + // A whitelist of renamable symbols. + if (llvm::isa<CXXRecordDecl>(RenameDecl)) ---------------- Why not a blacklist? I'd expect us to blacklist: - things with custom names (operators, constructors, etc) - virtual methods Probably some things are missing from the list, but fundamentally most that have names are pretty similar, right? ================ Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:132 + if (const auto *S = llvm::dyn_cast<CXXMethodDecl>(&RenameDecl)) { + // FIXME: renaming virtual methods requires to rename all overridens in + // subclasses, which our index doesn't support yet. ---------------- Isn't the same true for local rename? ================ Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:221 + if (!Index) + return {}; + RefsRequest RQuest; ---------------- This behavior is very surprising... Obviously, returning empty results is incorrect and the callers have to handle the no-index case in a custom manner. Maybe accept only non-null index and handle in the caller? ================ Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:235 + }); + return AffectedFiles; +} ---------------- Again, it looks like the function returns incorrect results and the callers should actually handle the case where `SymbolID` cannot be obtained in a custom manner. Maybe accept a `SymbolID` as a parameter instead of `NamedDecl*` and the callers handle this case gracefully? ================ Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:255 + // !positionToOffset is O(N), it is okay at the moment since we only + // process at most 100 references. + auto RangeOffset = toRangeOffset(Occurrence, InitialCode); ---------------- This is not true anymore, right? We should probably try getting to `O(N)` to avoid slowing things down Could be a FIXME for now, but have to fix it soon. ================ Comment at: clang-tools-extra/clangd/refactor/Rename.h:33 + llvm::StringRef MainFilePath; + llvm::StringRef MainFileCode; + ---------------- hokein wrote: > 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`. `SourceManager` has access to the input buffer. `StringRef MainFileCode = SM.getBufferData(SM.getMainFileID());` ================ 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 ---------------- hokein wrote: > 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). Does that mean we have three states of files that rename should be aware of? - Dirty buffer - Main file contents - Contents on disk That definitely looks very complicated... Is there a chance we could abstract it away and just have a single source of truth for file contents? I am thinking of a function that handles getting the file buffer by name, so we have all the complicated logic in one place. Would look something like: ``` std::function<std::string(PathRef /*Path*/)> GetFileContents; ``` Implementation could try to do the following in order: 1. try to get contents of the main file. 2. if failed, get contents from the dirty buffers. 3. if failed, get contents from disk. Could be a helper function accepting `RenameInputs`, but important to use it everywhere we get the file contents. 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