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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:69
+
+// Query the index to get other file where the Decl is referenced.
+llvm::Optional<std::string> getOtherRefFile(const NamedDecl &Decl,
----------------
...to find some other file...


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:98
+
+// Check the symbol Decl is renameable, details are:
+//   1. symbol declared in the main file (not a header) => allow
----------------
Can we move these steps next to the associated code? The separate lists can be hard to maintain.

e.g. 
```
// If the symbol is in the main file (which is not a header), we can rename it
if (DeclaredInMainFile && !MainFileIsHeader)
  ...
```


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:99
+// Check the symbol Decl is renameable, details are:
+//   1. symbol declared in the main file (not a header) => allow
+//   2. symbol declared in the header
----------------
(which is not a header)


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:107
+llvm::Optional<ReasonToReject>
+renamable(const NamedDecl &Decl, StringRef MainFile, const SymbolIndex *Index) {
+  auto &ASTCtx = Decl.getASTContext();
----------------
renamableWithinFile


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:110
+  const auto &SM = ASTCtx.getSourceManager();
+  bool MainFileIsHeader = llvm::sys::path::extension(MainFile) == ".h";
+  bool DeclaredInMainFile =
----------------
this isn't the right check - LangOpts::IsHeaderFile


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:115
+  bool DeclaredInHeader =
+      !DeclaredInMainFile || (DeclaredInMainFile && MainFileIsHeader);
+  if (!DeclaredInHeader)
----------------
can we avoid the double negative here? maybe just inline into the if


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:129
+    return ReasonToReject::NonIndexable; // Case 2.2
+  // query index for xrefs. (not support for now)
+  auto OtherFile = getOtherRefFile(Decl, MainFile, *Index);
----------------
I can't parse "not support for now" - not sure this comment is necessary, the whole point of this function (with its current signature) is to reject this.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:167
+    return llvm::make_error<llvm::StringError>(
+        llvm::formatv("reject to rename: {0}", Message(*Reject)),
+        llvm::inconvertibleErrorCode());
----------------
this is a user-visible error message, right?

- "reject to rename" doesn't have any obvious meaning to me. Maybe "Cannot rename symbol" ?
- "no index is provided" -> "symbol may be used in other files (no index available)"
- "the symbol is not indexable" -> "symbol may be used in other files (not eligible for indexing)"


================
Comment at: clang-tools-extra/clangd/refactor/Rename.h:21
 /// Occurrences outside the current file are not modified.
-llvm::Expected<tooling::Replacements> renameWithinFile(ParsedAST &AST,
-                                                       llvm::StringRef File,
-                                                       Position Pos,
-                                                       llvm::StringRef NewName);
+/// Only support renaming symbols not being used outside the file.
+llvm::Expected<tooling::Replacements>
----------------
It's uncler what "only support" means here.
"Renaming a symbol that's used in another file (per the index) returns an error"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63426