[PATCH] D88634: [clangd] Extend the rename API.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:408
       return CB(InpAST.takeError());
-    auto &AST = InpAST->AST;
-    const auto &SM = AST.getSourceManager();
-    auto Loc = sourceLocationInMainFile(SM, Pos);
-    if (!Loc)
-      return CB(Loc.takeError());
-    const auto *TouchingIdentifier =
-        spelledIdentifierTouching(*Loc, AST.getTokens());
-    if (!TouchingIdentifier)
-      return CB(llvm::None); // no rename on non-identifiers.
-
-    auto Range = halfOpenToRange(
-        SM, CharSourceRange::getCharRange(TouchingIdentifier->location(),
-                                          TouchingIdentifier->endLocation()));
-
-    if (RenameOpts.AllowCrossFile)
-      // FIXME: we now assume cross-file rename always succeeds, revisit this.
-      return CB(Range);
-
-    // Performing the local rename isn't substantially more expensive than
-    // doing an AST-based check, so we just rename and throw away the results.
-    auto Changes = clangd::rename({Pos, "dummy", AST, File, Index, RenameOpts,
-                                   /*GetDirtyBuffer=*/nullptr});
-    if (!Changes) {
+    clangd::FileIndex EmptyIndex;
+    // prepareRename is latency-sensitive:
----------------
the empty index is weird here - can we pass nullptr?
Currently nullptr leads to an error in the !crossfile case, but I think we can give rename(Index=nullptr, RenameOpts.CrossFile=true) the behavior we want.

(otherwise, if we need an empty index use MemIndex())


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:411
+    //  - for single-file rename, performing rename isn't substantially more
+    //    expensive than doing an AST-based check;
+    //  - for cross-file rename, we deliberately pass an empty index to save the
----------------
mention "the index is used to see if the local rename is complete"?


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:416
+    auto Results = clangd::rename(
+        {Pos, "dummy", InpAST->AST, File,
+         RenameOpts.AllowCrossFile ? &EmptyIndex : Index, RenameOpts});
----------------
we're now returning the "dummy" string to the caller, so we should document it somewhere (or ideally just make it the empty string and document that)


================
Comment at: clang-tools-extra/clangd/ClangdServer.h:277
+  ///
+  /// The returned result may be incomplete as it only contains occurrences from
+  /// the current main file.
----------------
nit: drop "may be incomplete as it"?


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:500
   if (!Opts.AllowCrossFile || RenameDecl.getParentFunctionOrMethod()) {
-    return FileEdits(
-        {std::make_pair(RInputs.MainFilePath,
-                        Edit{MainFileCode, std::move(*MainFileRenameEdit)})});
+    return RenameResults{
+        CurrentIdentifier,
----------------
nit: I'd find this more readable as a default construction followed by assignments to the fields


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:507
 
-  FileEdits Results;
+  FileEdits Edits;
   // Renameable safely guards us that at this point we are renaming a local
----------------
and again here - declare the whole struct first and fill it in?


================
Comment at: clang-tools-extra/clangd/refactor/Rename.h:58
 
+struct RenameResults {
+  // The range of the symbol that the user can attempt to rename.
----------------
nit: I'd suggest RenameResult singular, consistent with CodeCompleteResult and ReferencesResult


================
Comment at: clang-tools-extra/clangd/refactor/Rename.h:60
+  // The range of the symbol that the user can attempt to rename.
+  Range R;
+  FileEdits Edits;
----------------
Give this a real name... Target?


================
Comment at: clang-tools-extra/clangd/refactor/Rename.h:61
+  Range R;
+  FileEdits Edits;
+};
----------------
It's slightly awkward to have the half-populated state (may or may not contain cross-file results, can't tell without the query).

I'd consider redundantly including `Edit LocalChanges` and `FileEdits GlobalChanges` with the former always set and the latter empty when returned from preparerename.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88634