ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments.
================ Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:274 +llvm::Expected<FileEdits> +renameOutsideFile(const NamedDecl *RenameDecl, llvm::StringRef MainFilePath, + llvm::StringRef NewName, const SymbolIndex *Index, ---------------- hokein wrote: > ilya-biryukov wrote: > > NIT: Maybe call it `renameWithIndex` instead? > > It should capture what this function is doing better... > > > > But up to you > I would keep the current name, as it is symmetrical to the `renameWithinFile`. Yeah, those go together. Using `renameWithIndex` would mean the other one should be `renameWithAST` (or similar). Feel free to keep as is too. ================ Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:216 +// Return all rename occurrences (per the index) outside of the main file, +// grouped by the file name. +llvm::StringMap<std::vector<Range>> ---------------- NIT: grouped by the **absolute** file name? Or is there a better term than "absolute" to describe this? ================ Comment at: clang-tools-extra/clangd/refactor/Rename.h:23 +using DirtyBufferGetter = + std::function<llvm::Optional<std::string>(PathRef Path)>; ---------------- 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`? ================ Comment at: clang-tools-extra/clangd/refactor/Rename.h:24 +using DirtyBufferGetter = + std::function<llvm::Optional<std::string>(PathRef Path)>; + ---------------- NIT: maybe use `function_ref`? We definitely don't store this function anywhere. 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