[PATCH] D65936: [clangd] Use raw rename functions to implement the rename.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:175
+    // Currently, we only support normal rename (one range) for C/C++.
+    // FIXME: support multiple-range rename for objective-c methods.
+    if (Rename.getNameRanges().size() > 1)
----------------
hokein wrote:
> sammccall wrote:
> > is this a regression in this patch, or did the limitation already exist?
> this is not a regression, this is a limitation in clangd.
can you point me to where this behavior (bailing out when there's more than one name range) occurred in the old version of the code?

(by regression I would mean e.g. if objective-c selector renames worked before this change, but don't work afterwards. But either way, it'd be useful to understand why)


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:178
+      continue;
+    cantFail(FilteredChanges.add(tooling::Replacement(
+        AST.getASTContext().getSourceManager(),
----------------
hokein wrote:
> sammccall wrote:
> > why can't this fail?
> I just kept the previous behavior. Made the error emit to the client rather than crashing here.
I'm not sure what you meant by "kept the previous behavior".
Yes, before this change there was a call `cantFail`, but the implementation was different, so it's not clear the set of errors it is handling are the same.

Previously, I guess it couldn't fail because we assumed that the underlying layer (which returned a set of edits) wouldn't produce conflicting ones.

Here, we're producing the set of edits ourselves, so any global property (e.g. "no pair of these conflict") needs to be established by us. So we should document whether/under what circumstances it can fail.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:182
+            CharSourceRange::getCharRange(Rename.getNameRanges()[0]), NewName)))
+      return std::move(Err);
   }
----------------
for actual error handling behavior (if this can actually fail): is this a deliberate choice to refuse to perform any of the edits if there are conflicts? Is this better than applying some non-conflicting set?

Unlike most error-propagation, this is a nontrivial policy choice and needs a comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65936