[PATCH] D69263: [clangd] Implement cross-file rename.

[Ilya Biryukov via Phabricator via cfe-commits]

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