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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:79
+
+  if (Index) {
+    // Consult the index to determine whether the symbol is used outside of
----------------
pull out a function for this?


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:83
+    // FIXME: we may skip querying the index if D is function-local.
+    const NamedDecl *D =
+        clang::tooling::getNamedDeclAt(ASTCtx, SourceLocationBeg);
----------------
it's unfortunate that our "what's under the cursor" logic here has to duplicate what's done by RenameOccurrences::initiate().

Can we have RenameOccurrences expose the NamedDecl instead?


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:83
+    // FIXME: we may skip querying the index if D is function-local.
+    const NamedDecl *D =
+        clang::tooling::getNamedDeclAt(ASTCtx, SourceLocationBeg);
----------------
sammccall wrote:
> it's unfortunate that our "what's under the cursor" logic here has to duplicate what's done by RenameOccurrences::initiate().
> 
> Can we have RenameOccurrences expose the NamedDecl instead?
does this not work for macros?


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:85
+        clang::tooling::getNamedDeclAt(ASTCtx, SourceLocationBeg);
+    if (!D)
+      return llvm::make_error<llvm::StringError>(
----------------
We're effectively doing this test twice (see `if (!Rename)` below) and emitting different error mesages

maybe we should just skip the rest of the check in this case?
(Exposing the ND from the RenameOccurrences would help make this duplication more obvious)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63426