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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
