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> _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
