[clang-tools-extra] 6259016 - [clang-tidy] Fix false positive in readability-identifier-naming check involving override attribute

[Salman Javed via cfe-commits]

Author: Fabian Wolff
Date: 2021-11-19T22:31:11+13:00
New Revision: 6259016361345e09f0607ef4e037e00bcbe4bd40

URL: https://github.com/llvm/llvm-project/commit/6259016361345e09f0607ef4e037e00bcbe4bd40
DIFF: https://github.com/llvm/llvm-project/commit/6259016361345e09f0607ef4e037e00bcbe4bd40.diff

LOG: [clang-tidy] Fix false positive in readability-identifier-naming check involving override attribute

Overriding methods should not get a readability-identifier-naming
warning because the issue can only be fixed in the base class; but the
current check for whether a method is overriding does not take the
override attribute into account.

Differential Revision: https://reviews.llvm.org/D113830

Added: 
    

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

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp b/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
index d275b475f97c0..cfbe79c525942 100644
--- a/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -1257,7 +1257,7 @@ StyleKind IdentifierNamingCheck::findStyleKind(
 
   if (const auto *Decl = dyn_cast<CXXMethodDecl>(D)) {
     if (Decl->isMain() || !Decl->isUserProvided() ||
-        Decl->size_overridden_methods() > 0)
+        Decl->size_overridden_methods() > 0 || Decl->hasAttr<OverrideAttr>())
       return SK_Invalid;
 
     // If this method has the same name as any base method, this is likely

diff  --git a/clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
index 3622fe574a72b..01bcb34eadc0d 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
@@ -285,6 +285,10 @@ class AOverridden {
   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;
+
+  virtual void BadBaseMethodNoAttr() = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for virtual method 'BadBaseMethodNoAttr'
+  // CHECK-FIXES: {{^}}  virtual void v_Bad_Base_Method_No_Attr() = 0;
 };
 
 class COverriding : public AOverridden {
@@ -293,6 +297,9 @@ class COverriding : public AOverridden {
   void BadBaseMethod() override {}
   // CHECK-FIXES: {{^}}  void v_Bad_Base_Method() override {}
 
+  void BadBaseMethodNoAttr() /* override */ {}
+  // CHECK-FIXES: {{^}}  void v_Bad_Base_Method_No_Attr() /* override */ {}
+
   void foo() {
     BadBaseMethod();
     // CHECK-FIXES: {{^}}    v_Bad_Base_Method();
@@ -302,12 +309,79 @@ class COverriding : public AOverridden {
     // CHECK-FIXES: {{^}}    AOverridden::v_Bad_Base_Method();
     COverriding::BadBaseMethod();
     // CHECK-FIXES: {{^}}    COverriding::v_Bad_Base_Method();
+
+    BadBaseMethodNoAttr();
+    // CHECK-FIXES: {{^}}    v_Bad_Base_Method_No_Attr();
+    this->BadBaseMethodNoAttr();
+    // CHECK-FIXES: {{^}}    this->v_Bad_Base_Method_No_Attr();
+    AOverridden::BadBaseMethodNoAttr();
+    // CHECK-FIXES: {{^}}    AOverridden::v_Bad_Base_Method_No_Attr();
+    COverriding::BadBaseMethodNoAttr();
+    // CHECK-FIXES: {{^}}    COverriding::v_Bad_Base_Method_No_Attr();
   }
 };
 
-void VirtualCall(AOverridden &a_vItem) {
+// Same test as above, now with a dependent base class.
+template<typename some_t>
+class ATOverridden {
+public:
+  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;
+
+  virtual void BadBaseMethodNoAttr() = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for virtual method 'BadBaseMethodNoAttr'
+  // CHECK-FIXES: {{^}}  virtual void v_Bad_Base_Method_No_Attr() = 0;
+};
+
+template<typename some_t>
+class CTOverriding : public ATOverridden<some_t> {
+  // Overriding a badly-named base isn't a new violation.
+  // FIXME: The fixes from the base class should be propagated to the derived class here
+  //        (note that there could be specializations of the template base class, though)
+  void BadBaseMethod() override {}
+
+  // Without the "override" attribute, and due to the dependent base class, it is not
+  // known whether this method overrides anything, so we get the warning here.
+  virtual void BadBaseMethodNoAttr() {};
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for virtual method 'BadBaseMethodNoAttr'
+  // CHECK-FIXES: {{^}}  virtual void v_Bad_Base_Method_No_Attr() {};
+};
+
+template<typename some_t>
+void VirtualCall(AOverridden &a_vItem, ATOverridden<some_t> &a_vTitem) {
+  a_vItem.BadBaseMethod();
+  // CHECK-FIXES: {{^}}  a_vItem.v_Bad_Base_Method();
+
+  // FIXME: The fixes from ATOverridden should be propagated to the following call
+  a_vTitem.BadBaseMethod();
+}
+
+// Same test as above, now with a dependent base class that is instantiated below.
+template<typename some_t>
+class ATIOverridden {
+public:
+  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;
+};
+
+template<typename some_t>
+class CTIOverriding : public ATIOverridden<some_t> {
+public:
+  // Overriding a badly-named base isn't a new violation.
+  void BadBaseMethod() override {}
+  // CHECK-FIXES: {{^}}  void v_Bad_Base_Method() override {}
+};
+
+template class CTIOverriding<int>;
+
+void VirtualCallI(ATIOverridden<int>& a_vItem, CTIOverriding<int>& a_vCitem) {
   a_vItem.BadBaseMethod();
   // CHECK-FIXES: {{^}}  a_vItem.v_Bad_Base_Method();
+
+  a_vCitem.BadBaseMethod();
+  // CHECK-FIXES: {{^}}  a_vCitem.v_Bad_Base_Method();
 }
 
 template <typename derived_t>