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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:219
+  // us whether the ref results is completed.
+  RQuest.Limit = 100;
+  if (auto ID = getSymbolID(RenameDecl))
----------------
ilya-biryukov wrote:
> Why do we need to limit the number of references in the first place?
I was thinking this is for performance, if the index returns too many results (the number of affected files >> our limit), we will ends up doing many unnecessary work (like resolving URIs).  Removed it now, it seems a premature optimization, we can revisit this afterwards.


================
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,
----------------
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`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69263/new/

https://reviews.llvm.org/D69263