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

Haojian Wu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Aug 8 06:48:44 PDT 2019


hokein marked 3 inline comments as done.
hokein 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)
----------------
sammccall wrote:
> 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)
There is a [FIXME](https://github.com/llvm/llvm-project/blob/master/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp#L219) states that. 

> 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?

I'm pretty sure that  the old version of the code will trigger the [assertion](https://github.com/llvm/llvm-project/blob/master/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp#L151) when doing multiple-piece rename, as the name pieces of the `NewName` is always 1 according to the [code](https://github.com/llvm/llvm-project/blob/master/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp#L221).


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:178
+      continue;
+    cantFail(FilteredChanges.add(tooling::Replacement(
+        AST.getASTContext().getSourceManager(),
----------------
sammccall wrote:
> 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.
> 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.

This is true, at this point we shouldn't see conflicts, because if we have conflicts, we have caught an [error](https://github.com/llvm/llvm-project/blob/master/clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp#L155)  (in the RefactoringResultConsumer::handleError) when invoking the `RenameOccurrences`.

To sum up, in the previous version, we will report the error and refuse to apply any edits if there are conflicts, which is the same behavior we keep in current patch.
The only difference I see is that we traversal main file decls (rather than TUDecl), if we have bugs in the main file decls, we may generate conflicting edits, but I wouldn't too worry about this (as top level decls are a small subset of decls inside TUDecl).






================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:182
+            CharSourceRange::getCharRange(Rename.getNameRanges()[0]), NewName)))
+      return std::move(Err);
   }
----------------
sammccall wrote:
> 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.
I think if there are conflicts, it is a signal that indicates we have bugs in the code (either in clangd, or rename library), applying parts of them may be not valuable (like generating uncompilable code)

I'd prefer to refuse to perform renaming if we have conflicts, WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65936





More information about the cfe-commits mailing list