[PATCH] D72638: [clangd] Fix rename for explicit destructor calls

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev created this revision.
kbobyrev added a reviewer: sammccall.
Herald added subscribers: usaxena95, kadircet, arphaman, jkorous, MaskRay, ilya-biryukov.
Herald added a project: clang.
kbobyrev edited the summary of this revision.
kbobyrev added a comment.

I'm not sure if leaving both `ReferenceLoc`s pointing to the same location is a sensible solution, but merging them seems quite complicated and probably not really worth the effort. I've considered multiple alternative solutions, as described in https://github.com/clangd/clangd/issues/236 and converged to the one presented in this patch.


When triggering rename of the class name in the code with explicit destructor
calls, rename fails. Consider the following piece of code:

  class Foo;
  
  ...
  
  Foo f;
  f.~/*...*/Foo();

`findExplicitReferences` will report two `ReferenceLoc` for destructor call:
one is comming from `MemberExpr` (i.e. destructor call itself) and would point
to the tilde:

  f.~/*...*/Foo();
    ^

And the second one is pointing to the typename and is coming from `TypeLoc`.

  f.~/*...*/Foo();
            ^

This causes rename to produce incorrect textual replacements. This patch
updates `MemberExpr` handler to detect destructor calls and advances the
reported location to the beginning of typename:

  f.~/*...*/Foo();
            ^

Resolves: https://github.com/clangd/clangd/issues/236


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72638

Files:
  clang-tools-extra/clangd/FindTarget.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp


Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -265,6 +265,22 @@
         }
       )cpp",
 
+      // Destructor.
+      R"cpp(
+        class [[F^oo]] {
+        public:
+          ~[[^Foo]]();
+        };
+
+        [[Foo^]]::~[[^Foo]]() {}
+
+        int main() {
+          [[Fo^o]] f;
+          f.~/*something*/[[^Foo]]();
+          f.~[[^Foo]]();
+        }
+      )cpp",
+
       // CXXConstructor initializer list.
       R"cpp(
         class Baz {};
Index: clang-tools-extra/clangd/FindTarget.cpp
===================================================================
--- clang-tools-extra/clangd/FindTarget.cpp
+++ clang-tools-extra/clangd/FindTarget.cpp
@@ -596,8 +596,30 @@
     }
 
     void VisitMemberExpr(const MemberExpr *E) {
+      auto NameLocation = E->getMemberNameInfo().getLoc();
+      // If E is the destructor call, then NameLocation would point to the
+      // location of the tilde, i.e.
+      //
+      //   Foo f;
+      //   f.~/*something*/Foo();
+      //     ^
+      //   f.~Foo();
+      //     ^
+      //
+      // When renaming, this would cause incorrect behavior. We consider the
+      // location of the type name to be the right one (this is also the
+      // location used to trigger the rename itself, triggering rename with
+      // cursor at `~` won't work.
+      //
+      //   Foo f;
+      //   f.~/*something*/Foo();
+      //                   ^
+      //   f.~Foo();
+      //      ^
+      if (llvm::dyn_cast<CXXDestructorDecl>(E->getMemberDecl()))
+        NameLocation = E->getEndLoc();
       Refs.push_back(ReferenceLoc{E->getQualifierLoc(),
-                                  E->getMemberNameInfo().getLoc(),
+                                  NameLocation,
                                   /*IsDecl=*/false,
                                   {E->getFoundDecl()}});
     }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D72638.237749.patch
Type: text/x-patch
Size: 2034 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200113/d46d0f28/attachment.bin>