[PATCH] D66864: [clang-tidy] readability-identifier-naming shouldn't complain about CRTP pseudo-overrides

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, xazax.hun.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66864

Files:
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
  clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst
  clang-tools-extra/test/clang-tidy/readability-identifier-naming.cpp


Index: clang-tools-extra/test/clang-tidy/readability-identifier-naming.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/readability-identifier-naming.cpp
+++ clang-tools-extra/test/clang-tidy/readability-identifier-naming.cpp
@@ -262,6 +262,32 @@
   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'
+};
+
+class COverriding : public AOverridden {
+public:
+  // Overriding a badly-named base isn't a new violation.
+  void BadBaseMethod() override {}
+};
+
+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> {
+public:
+  // Hiding a badly-named base isn't a new violation.
+  double BadBaseMethod(double) { return 0; }
+};
+
 template<typename T>
 // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: invalid case style for type template parameter 'T'
 // CHECK-FIXES: {{^}}template<typename t_t>{{$}}
Index: clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst
+++ clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst
@@ -26,6 +26,10 @@
 different rules for different kinds of identifiers. In general, the rules are
 falling back to a more generic rule if the specific case is not configured.
 
+The naming of virtual methods is reported where they occur in the base class,
+but not where they are overridden, as it can't be fixed locally there.
+This also applies for pseudo-override patterns like CRTP.
+
 Options
 -------
 
Index: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -10,6 +10,7 @@
 
 #include "../utils/ASTUtils.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/AST/CXXInheritance.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Preprocessor.h"
@@ -579,6 +580,15 @@
         Decl->size_overridden_methods() > 0)
       return SK_Invalid;
 
+    // If this method has the same name as any base method, this is likely
+    // necessary even if it's not an override. e.g. CRTP.
+    auto FindHidden = [&](const CXXBaseSpecifier *S, clang::CXXBasePath &P) {
+      return CXXRecordDecl::FindOrdinaryMember(S, P, Decl->getDeclName());
+    };
+    CXXBasePaths UnusedPaths;
+    if (Decl->getParent()->lookupInBases(FindHidden, UnusedPaths))
+      return SK_Invalid;
+
     if (Decl->isConstexpr() && NamingStyles[SK_ConstexprMethod])
       return SK_ConstexprMethod;
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D66864.217584.patch
Type: text/x-patch
Size: 3118 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190828/1e3d4026/attachment.bin>