[clang-tidy] Better support for Override function in RenamerClangTidy based checks

[njames93]

Summary

Methods that override virtual methods will now get renamed if the initial virtual method has a name violation.
Addresses https://bugs.llvm.org/show_bug.cgi?id=34879

[alexfh]

Looks good! Thanks for the fix! IIUC, this is related to https://bugs.llvm.org/show_bug.cgi?id=34879? Makes sense to specify this in the patch description.

[alexfh]

Feel free to ping patches every week or so. It looks like in this case all the reviewers were swamped with something else at the time.

[njames93]

Updated release notes to reflect change

clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp

 
 void RenamerClangTidyCheck::registerPPCallbacks(
     const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
   ModuleExpanderPP->addPPCallbacks(
       std::make_unique<RenamerClangTidyCheckPPCallbacks>(ModuleExpanderPP,
                                                          this));
 }
 
+/// Returns the function that \p Method is overridding. If There are none or
+/// multiple overrides it returns nullptr. If the overridden function itself is
+/// overridding then it will recurse up to find the first decl of the function.
+static const CXXMethodDecl *getOverrideMethod(const CXXMethodDecl *Method) {
+  if (Method->size_overridden_methods() != 1)
+    return nullptr;
+  while (true) {
+    Method = *Method->begin_overridden_methods();
+    assert(Method && "Overridden method shouldn't be null");
+    unsigned NumOverrides = Method->size_overridden_methods();
+    if (NumOverrides == 0)
+      return Method;
+    if (NumOverrides > 1)
+      return nullptr;
+  }
+}
+
 void RenamerClangTidyCheck::addUsage(
     const RenamerClangTidyCheck::NamingCheckId &Decl, SourceRange Range,
     SourceManager *SourceMgr) {
   // Do nothing if the provided range is invalid.
   if (Range.isInvalid())
     return;
 
   // If we have a source manager, use it to convert to the spelling location for
if (SourceMgr && SourceMgr->isWrittenInScratchSpace(FixLocation))
     Failure.FixStatus = RenamerClangTidyCheck::ShouldFixStatus::InsideMacro;
 
   if (!utils::rangeCanBeFixed(Range, SourceMgr))
     Failure.FixStatus = RenamerClangTidyCheck::ShouldFixStatus::InsideMacro;
 }
 
 void RenamerClangTidyCheck::addUsage(const NamedDecl *Decl, SourceRange Range,
                                      SourceManager *SourceMgr) {
+  if (const auto *Method = dyn_cast<CXXMethodDecl>(Decl)) {
+    if (const CXXMethodDecl *Overridden = getOverrideMethod(Method))
+      Decl = Overridden;
+  }
   Decl = cast<NamedDecl>(Decl->getCanonicalDecl());
   return addUsage(RenamerClangTidyCheck::NamingCheckId(Decl->getLocation(),
                                                        Decl->getNameAsString()),
                   Range, SourceMgr);
 }
 
 const NamedDecl *findDecl(const RecordDecl &RecDecl, StringRef DeclName) {
   for (const Decl *D : RecDecl.decls()) {
if (const auto *Value = Result.Nodes.getNodeAs<FunctionDecl>("decl")) {
       for (const ParmVarDecl *Param : Value->parameters()) {
         if (const TypedefType *Typedef =
                 Param->getType().getTypePtr()->getAs<TypedefType>())
           addUsage(Typedef->getDecl(), Value->getSourceRange(),
                    Result.SourceManager);
       }
     }
 
+    // Fix overridden methods
+    if (const auto *Method = Result.Nodes.getNodeAs<CXXMethodDecl>("decl")) {
+      if (const CXXMethodDecl *Overridden = getOverrideMethod(Method)) {
+        addUsage(Overridden, Method->getLocation());
+        return; // Don't try to add the actual decl as a Failure.
+      }
+    }
+
     // Ignore ClassTemplateSpecializationDecl which are creating duplicate
     // replacements with CXXRecordDecl.
     if (isa<ClassTemplateSpecializationDecl>(Decl))
       return;
 
     Optional<FailureInfo> MaybeFailure =
         GetDeclFailureInfo(Decl, *Result.SourceManager);
     if (!MaybeFailure)

clang-tools-extra/docs/ReleaseNotes.rst

- Improved :doc:`modernize-loop-convert
   Now able to transform iterator loops using ``rbegin`` and ``rend`` methods.
 
 - Improved :doc:`readability-identifier-naming
   <clang-tidy/checks/readability-identifier-naming>` check.
 
   Added an option `GetConfigPerFile` to support including files which use
   different naming styles.
 
+  Now renames overridden virtual methods if the method they override has a
+  style violation.
+
 - Removed `google-runtime-references` check because the rule it checks does
   not exist in the Google Style Guide anymore.
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 

clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp

void InstantiateClassMethods() {
   CMyWellNamedClass2<int> x5(42, nullptr);
 }
 
 class AOverridden {
 public:
   virtual ~AOverridden() = default;
   virtual void BadBaseMethod() = 0;
   // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for virtual method 'BadBaseMethod'
+  // CHECK-FIXES: {{^}}  virtual void v_Bad_Base_Method() = 0;
 };
 
 class COverriding : public AOverridden {
 public:
   // Overriding a badly-named base isn't a new violation.
   void BadBaseMethod() override {}
+  // CHECK-FIXES: {{^}}  void v_Bad_Base_Method() override {}
+  
+  void foo() {
+    BadBaseMethod();
+    // CHECK-FIXES: {{^}}    v_Bad_Base_Method();
+    this->BadBaseMethod();
+    // CHECK-FIXES: {{^}}    this->v_Bad_Base_Method();
+    AOverridden::BadBaseMethod();
+    // CHECK-FIXES: {{^}}    AOverridden::v_Bad_Base_Method();
+    COverriding::BadBaseMethod();
+    // CHECK-FIXES: {{^}}    COverriding::v_Bad_Base_Method();
+  }
 };
 
+void VirtualCall(AOverridden &a_vItem) {
+  a_vItem.BadBaseMethod();
+  // CHECK-FIXES: {{^}}  a_vItem.v_Bad_Base_Method();
+}
+
 template <typename derived_t>
 class CRTPBase {
 public:
   void BadBaseMethod(int) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for method 'BadBaseMethod'
 };
 
 class CRTPDerived : CRTPBase<CRTPDerived> {