[clang-rename] add support for overridden functions

[omtcyfz]

Summary

add implementation and tests for virtual function renaming

[Prazek]

haven't found anything else

clang-rename/USRFindingAction.cpp

62

const auto &OverridenMethod

[omtcyfz]

clang-rename/USRFindingAction.cpp

62

Fixed; thanks!

[klimek]

clang-rename/USRFindingAction.cpp

12

... at <offset>, as well as all relevant USRs.

60

I'd use early exit here:
if (!D->isVirtual) return true;
...

62

This is n^3, right? (for each virtual method we run over all overrides, searching through all USRs)
a) would be cool if we could use matchers for this, as they automatically memoize; if we don't want to do that, we might want to memoize ourselves
b) sorting USRs and using binary_search should be a cheap speed-up

144

Having all side-effects in the constructor is really unexpected to me. I'd add a Find() method.

[omtcyfz]

replaced RecursiveASTVisitor with ASTMatcher

[omtcyfz]

clang-rename/USRFindingAction.cpp

109

Thanks for the feedback!
a) done
b) using std::set instead and pushing into std::vector afterwards. I thing it's simple and efficient, too.

[klimek]

clang-rename/USRFindingAction.cpp

106

Still pondering whether 'relevant' is a good name here...

133

That way, the matchers don't actually get us too much - I'd hoped we already had an hasOverriddenMethod matcher (apparently we don't). It might make sense to implement one.
Then we can implement a matchesUSR matcher, too (which takes a set of USRs), and all those lookups would be cached automatically.
Fine to add a FIXME for this patch though.

147

Perhaps call this addUSRsFromOverrideSets or something? addRelevantUSRs doesn't really tell me anything about what it does, and it's currently pretty specific.

[omtcyfz]

Addressed Manuel's comments.

[klimek]

lg

clang-rename/USRFindingAction.cpp

 //===--- tools/extra/clang-rename/USRFindingAction.cpp - Clang rename tool ===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
-/// \brief Provides an action to rename every symbol at a point.
+/// \brief Provides an action to find USR for the symbol at <offset> and all
+/// relevant USRs aswell.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "USRFindingAction.h"
 #include "USRFinder.h"
 #include "clang/AST/AST.h"
 #include "clang/AST/ASTConsumer.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendAction.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/Refactoring.h"
 #include "clang/Tooling/Tooling.h"
+#include <algorithm>
 #include <string>
 #include <vector>
 
 using namespace llvm;
 
 namespace clang {
 namespace rename {
 
-// Get the USRs for the constructors of the class.
-static std::vector<std::string> getAllConstructorUSRs(
-    const CXXRecordDecl *Decl) {
-  std::vector<std::string> USRs;
-
-  // We need to get the definition of the record (as opposed to any forward
-  // declarations) in order to find the constructor and destructor.
-  const auto *RecordDecl = Decl->getDefinition();
+namespace {
+// \brief NamedDeclFindingConsumer should delegate finding USRs relevant to
+// given Decl to RelevantUSRFinder.
+//
+// FIXME: It's better to match ctors/dtors via typeLoc's instead of adding
+// their USRs to the storage, because we can also match CXXConversionDecl's by
+// typeLoc and we won't have to "manually" handle them here.
+class RelevantUSRFinder : public clang::RecursiveASTVisitor<RelevantUSRFinder> {
+public:
+  explicit RelevantUSRFinder(const Decl *FoundDecl, TranslationUnitDecl *TUDecl,
+                             std::vector<std::string> *USRs)
+      : FoundDecl(FoundDecl), USRs(USRs) {
+    USRs->push_back(getUSRForDecl(FoundDecl));
+    addRelevantUSRs();
+    TraverseDecl(TUDecl);
+  }
 
-  // Iterate over all the constructors and add their USRs.
-  for (const auto *CtorDecl : RecordDecl->ctors())
-    USRs.push_back(getUSRForDecl(CtorDecl));
+  // CXXMethodDecl which overrides one of the methods stored for renaming
+  // should be added to the storage, too.
+  bool VisitCXXMethodDecl(CXXMethodDecl *D) {
+    if (D->isVirtual()) {
+      bool Found = false;
+      for (auto &OverriddenMethod : D->overridden_methods()) {
+        if (std::find(USRs->begin(), USRs->end(),
+                      getUSRForDecl(OverriddenMethod)) != USRs->end()) {
+          Found = true;
+        }
+      }
+      if (Found) {
+        USRs->push_back(getUSRForDecl(D));
+      }
+    }
+    return true;
+  }
 
-  // Ignore destructors. GetLocationsOfUSR will find the declaration of and
-  // explicit calls to a destructor through TagTypeLoc (and it is better for the
-  // purpose of renaming).
+private:
+  void addRelevantUSRs() {
+    // If D is CXXRecordDecl we should add all USRs of its constructors.
+    if (const auto *RecordDecl = dyn_cast<CXXRecordDecl>(FoundDecl)) {
+      // Ignore destructors. Find the declaration of and explicit calls to a
+      // destructor through TagTypeLoc (and it is better for the purpose of
+      // renaming).
       //
       // For example, in the following code segment,
       //  1  class C {
       //  2    ~C();
       //  3  };
-  // At line 3, there is a NamedDecl starting from '~' and a TagTypeLoc starting
-  // from 'C'.
+      //
+      // At line 3, there is a NamedDecl starting from '~' and a TagTypeLoc
+      // starting from 'C'.
+      RecordDecl = RecordDecl->getDefinition();
 
-  return USRs;
+      // Iterate over all the constructors and add their USRs.
+      for (const auto *CtorDecl : RecordDecl->ctors()) {
+        USRs->push_back(getUSRForDecl(CtorDecl));
+      }
     }
+    // If D is CXXMethodDecl we should add all USRs of its overriden methods.
+    if (const auto *MethodDecl = dyn_cast<CXXMethodDecl>(FoundDecl)) {
+      for (auto &OverriddenMethod : MethodDecl->overridden_methods()) {
+        USRs->push_back(getUSRForDecl(OverriddenMethod));
+      }
+    }
+  }
+
+  const Decl *FoundDecl;
+  std::vector<std::string> *USRs;
+};
+} // namespace
 
 struct NamedDeclFindingConsumer : public ASTConsumer {
   void HandleTranslationUnit(ASTContext &Context) override {
     const auto &SourceMgr = Context.getSourceManager();
     // The file we look for the USR in will always be the main source file.
-    const auto Point = SourceMgr.getLocForStartOfFile(
-        SourceMgr.getMainFileID()).getLocWithOffset(SymbolOffset);
+    const auto Point = SourceMgr.getLocForStartOfFile(SourceMgr.getMainFileID())
+                           .getLocWithOffset(SymbolOffset);
     if (!Point.isValid())
       return;
     const NamedDecl *FoundDecl = nullptr;
     if (OldName.empty()) {
       FoundDecl = getNamedDeclAt(Context, Point);
     } else {
       FoundDecl = getNamedDeclFor(Context, OldName);
     }
     if (FoundDecl == nullptr) {
       FullSourceLoc FullLoc(Point, SourceMgr);
       errs() << "clang-rename: could not find symbol at "
              << SourceMgr.getFilename(Point) << ":"
              << FullLoc.getSpellingLineNumber() << ":"
              << FullLoc.getSpellingColumnNumber() << " (offset " << SymbolOffset
              << ").\n";
       return;
     }
 
-    // If the decl is a constructor or destructor, we want to instead take the
-    // decl of the parent record.
-    if (const auto *CtorDecl = dyn_cast<CXXConstructorDecl>(FoundDecl))
+    // If FoundDecl is a constructor or destructor, we want to instead take the
+    // Decl of the corresponding class.
+    if (const auto *CtorDecl = dyn_cast<CXXConstructorDecl>(FoundDecl)) {
       FoundDecl = CtorDecl->getParent();
-    else if (const auto *DtorDecl = dyn_cast<CXXDestructorDecl>(FoundDecl))
+    } else if (const auto *DtorDecl = dyn_cast<CXXDestructorDecl>(FoundDecl)) {
       FoundDecl = DtorDecl->getParent();
-
-    // If the decl is in any way relatedpp to a class, we want to make sure we
-    // search for the constructor and destructor as well as everything else.
-    if (const auto *Record = dyn_cast<CXXRecordDecl>(FoundDecl))
-      *USRs = getAllConstructorUSRs(Record);
-
-    USRs->push_back(getUSRForDecl(FoundDecl));
+    }
     *SpellingName = FoundDecl->getNameAsString();
+
+    auto *TUDecl = Context.getTranslationUnitDecl();
+    RelevantUSRFinder Finder(FoundDecl, TUDecl, USRs);
   }
 
   unsigned SymbolOffset;
   std::string OldName;
   std::string *SpellingName;
   std::vector<std::string> *USRs;
 };
 
-std::unique_ptr<ASTConsumer>
-USRFindingAction::newASTConsumer() {
+std::unique_ptr<ASTConsumer> USRFindingAction::newASTConsumer() {
   std::unique_ptr<NamedDeclFindingConsumer> Consumer(
       new NamedDeclFindingConsumer);
   SpellingName = "";
   Consumer->SymbolOffset = SymbolOffset;
   Consumer->OldName = OldName;
   Consumer->USRs = &USRs;
   Consumer->SpellingName = &SpellingName;
   return std::move(Consumer);
 }
 
 } // namespace rename
 } // namespace clang

test/clang-rename/VirtualFunctionFindInBaseClass.cpp

+// RUN: cat %s > %t.cpp
+// RUN: clang-rename -offset=165 -new-name=bar %t.cpp -i --
+// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
+
+class Foo {
+public:
+  virtual void foo() {}   // CHECK: virtual void bar() {}
+};
+
+class Bar : public Foo {
+public:
+  void foo() override {}  // CHECK: void bar() override {}
+};
+
+class Baz : public Bar {
+ public:
+  void foo() override {}  // CHECK: void bar() override {}
+};
+
+int main() {
+  Foo f;
+  f.foo();                // CHECK: f.bar();
+
+  Bar b;
+  b.foo();                // CHECK: b.bar();
+
+  Baz obj;
+  obj.foo();              // CHECK: obj.bar();
+
+  return 0;
+}
+
+// Use grep -FUbo 'foo' <file> to get the correct offset of foo when changing

test/clang-rename/VirtualFunctionFindInDerivedClass.cpp

+// RUN: cat %s > %t.cpp
+// RUN: clang-rename -offset=165 -new-name=bar %t.cpp -i --
+// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
+
+class Foo {
+public:
+  virtual void foo() {}   // CHECK: virtual void bar() {}
+};
+
+class Bar : public Foo {
+public:
+  void foo() override {}  // CHECK: void bar() override {}
+};
+
+class Baz : public Bar {
+ public:
+  void foo() override {}  // CHECK: void bar() override {}
+};
+
+int main() {
+  Foo f;
+  f.foo();                // CHECK: f.bar();
+
+  Bar b;
+  b.foo();                // CHECK: b.bar();
+
+  Baz obj;
+  obj.foo();              // CHECK: obj.bar();
+
+  return 0;
+}
+
+// Use grep -FUbo 'foo' <file> to get the correct offset of foo when changing