clang-rename: fix renaming member functions

[vmiklos]

Summary

Testcase by Kirill Bobyrev.

[omtcyf0]

This one doesn't fix it though.

It only deals with declarations of overridden functions.

This is the test that still fails:

/ RUN: cat %s > %t.cpp
// RUN: clang-rename -offset=161 -new-name=boo %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
// XFAIL: *

class A {
public:
  virtual void foo() {}
};

class B : public A {
public:
  void foo() override {}  // OK: B::foo overrides A::foo
};

int main() {
  A a;
  a.foo();

  B b;
  b.foo();

  return 0;
}

// Use grep -FUbo 'Cla' <file> to get the correct offset of foo when changing
// this file.

[vmiklos]

Forgot to add asserts for main() in the testcase.

[omtcyf0]

Hi!

Sorry for a late response.

The issue is that you only deal with the consequences of this "bug". Overridden functions simply have different USRs, so the solution is just to add relevant USRs. Trying to deal with it in USRLocFinder.cpp doesn't seem right; the chances of dealing with such issue correctly on that side are very small. I.e. it still doesn't pass the more complicated test I have in my patch (see below).

There's a patch I've been working on lately, it does exactly that. https://reviews.llvm.org/D22408 (it's from the other account I'll have to use from now, nvm)

[vmiklos]

I see your point, then let me abandon this.

clang-rename/USRLocFinder.cpp

if (getUSRForDecl(DestructorDecl->getParent()) == USR) {
         // Handles "Foo" from "Foo::~Foo".
         LocationsFound.push_back(DestructorDecl->getLocStart());
       }
     }
 
     return true;
   }
 
+  bool VisitCXXMethodDecl(CXXMethodDecl *Decl) {
+    handleCXXMethodDecl(Decl, Decl->getLocation());
+    return true;
+  }
+
   // Expression visitors:
 
   bool VisitDeclRefExpr(const DeclRefExpr *Expr) {
     const auto *Decl = Expr->getFoundDecl();
 
     checkNestedNameSpecifierLoc(Expr->getQualifierLoc());
     if (getUSRForDecl(Decl) == USR) {
       const SourceManager &Manager = Decl->getASTContext().getSourceManager();
public:
   bool VisitCXXReinterpretCastExpr(clang::CXXReinterpretCastExpr *Expr) {
     return handleCXXNamedCastExpr(Expr);
   }
 
   bool VisitCXXConstCastExpr(clang::CXXConstCastExpr *Expr) {
     return handleCXXNamedCastExpr(Expr);
   }
 
+  bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *Expr) {
+    CXXMethodDecl *Decl = Expr->getMethodDecl();
+    handleCXXMethodDecl(Decl, Expr->getExprLoc());
+    return true;
+  }
+
   // Non-visitors:
 
   // \brief Returns a list of unique locations. Duplicate or overlapping
   // locations are erroneous and should be reported!
   const std::vector<clang::SourceLocation> &getLocationsFound() const {
     return LocationsFound;
   }
 
if (Decl && getUSRForDecl(Decl) == USR) {
       SourceLocation Location =
           Expr->getTypeInfoAsWritten()->getTypeLoc().getBeginLoc();
       LocationsFound.push_back(Location);
     }
 
     return true;
   }
 
+  void handleCXXMethodDecl(const CXXMethodDecl* Decl, SourceLocation Location) {
+    if (getUSRForDecl(Decl) == USR) {
+      // This member function was requested to be renamed explicitly.
+      LocationsFound.push_back(Location);
+    }
+    else if (Decl->isVirtual()) {
+      for (auto *OverridenDecl : Decl->overridden_methods()) {
+        if (getUSRForDecl(OverridenDecl) == USR) {
+          // This member function overwrites one that is to be renamed.
+          LocationsFound.push_back(Location);
+        }
+      }
+    }
+  }
+
   // All the locations of the USR were found.
   const std::string USR;
   // Old name that is renamed.
   const std::string PrevName;
   std::vector<clang::SourceLocation> LocationsFound;
 };
 } // namespace
 

test/clang-rename/VirtualFunction.cpp

+// RUN: cat %s > %t.cpp
+// RUN: clang-rename -offset=163 -new-name=boo %t.cpp -i --
+// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
+
+class A {
+public:
+  virtual void foo() { } // CHECK: virtual void boo() { }
+};
+
+class B : public A {
+public:
+  void foo() { } // CHECK: void boo() { }
+};
+
+int main() {
+  A a;
+  a.foo(); // CHECK: a.boo();
+
+  B b;
+  b.foo(); // CHECK: b.boo();
+
+  return 0;
+}
+
+// Use grep -FUbo 'foo' <file> to get the correct offset of foo when changing
+// this file.