[PATCH] D63426: [clangd] Narrow rename to local symbols.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for exposing the ND, cleaner.
Found another problem :-( not in your code but about this whole approach to using the index.
Let's talk tomorrow



================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:81
+  Req.Limit = 100;
+  if (auto ID = getSymbolID(&Decl))
+    Req.IDs.insert(*ID);
----------------
What about cases where the symbol has a USR but is not indexed? Non-toplevel, template bits we ignore, etc?

Not sure what we should do honestly. The "real" solution is probably to use QNames instead of symbol id here, and index then all. Silently allowing the rename seems pretty bad and needs at least to be explicit


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:113
+  assert(RenameDecl && "symbol must be found at this point");
+  // FIXME: we may skip querying the index if RenameDecl is function-local.
+  if (auto OtherFile = getOtherRefFile(*RenameDecl, File, Index))
----------------
This would be fixed in the helper function, move the fixme there


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63426