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

[Salman Javed via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG625901636134: [clang-tidy] Fix false positive in 
readability-identifier-naming check… (authored by fwolff, committed by 
salman-javed-nz).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113830/new/

https://reviews.llvm.org/D113830

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

Index: clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
@@ -285,6 +285,10 @@
   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 @@
   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 @@
 // 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 _vItem) {
+// Same test as above, now with a dependent base class.
+template
+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
+class CTOverriding : public ATOverridden {
+  // 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
+void VirtualCall(AOverridden _vItem, ATOverridden _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
+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
+class CTIOverriding : public ATIOverridden {
+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;
+
+void VirtualCallI(ATIOverridden& a_vItem, CTIOverriding& 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 
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

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

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz accepted this revision.
salman-javed-nz added a comment.
This revision is now accepted and ready to land.

LGTM. Nothing more to suggest from my side. Can we allow a few days for the 
other reviewers to put in their 2c.

As for the Bugzilla ticket , I'd 
say you've done enough to satisfy the primary grievance expressed in the PR: 
warnings should be raised in the base class only.
This is in line with the check's state purpose in the documentation 
.

> 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.

The comments below the PR talk about the need to propagate naming fixes to the 
derived classes and call sites. The last comment shows an example where fixes 
are not propagated at all, not even for the simple (non-template) case. But 
your tests for 
`COverriding` and `CTIOverriding` show that the replacements are indeed 
working. In any case, applying fixes outside the base class is going above and 
beyond the check's scope, and as you say, could lead to incorrect suggestions 
if the code is even slightly less trivial. More work could be done on this 
class, but doesn't have to be in this patch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113830/new/

https://reviews.llvm.org/D113830

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fabian Wolff via Phabricator via cfe-commits]

fwolff updated this revision to Diff 387401.
fwolff added a comment.

Thanks again for your feedback @salman-javed-nz! I think I've addressed all of 
your comments now.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113830/new/

https://reviews.llvm.org/D113830

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

Index: clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
@@ -285,6 +285,10 @@
   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 @@
   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 @@
 // 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 _vItem) {
+// Same test as above, now with a dependent base class.
+template
+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
+class CTOverriding : public ATOverridden {
+  // 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
+void VirtualCall(AOverridden _vItem, ATOverridden _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
+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
+class CTIOverriding : public ATIOverridden {
+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;
+
+void VirtualCallI(ATIOverridden& a_vItem, CTIOverriding& 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 
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
@@ -1257,7 +1257,7 @@
 
   if (const auto *Decl = dyn_cast(D)) {
 if (Decl->isMain() || 

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

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp:299
   // CHECK-FIXES: {{^}}  void v_Bad_Base_Method() override {}
+  void BadBaseMethodNoAttr() {}
+  // CHECK-FIXES: {{^}}  void v_Bad_Base_Method_No_Attr() {}

I think this line could spell out more clearly that it is testing the case 
where the `override` keyword is omitted. I don't think the `NoAttr()` suffix 
says enough. `override` is not the only Attr.

Possible solutions:
- You could incorporate the word "override" in the method name e.g. 
`BadBaseMethodNoOverride()`, or
- You could put a `/* override */` where `override` normally would be to bring 
attention to the fact that it is missing, or
- You could add a comment explaining what's going on, like the `// Overriding a 
badly-named base isn't a new violation` a couple of lines above.

Feel free to do what you think is the least janky.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp:299
   // CHECK-FIXES: {{^}}  void v_Bad_Base_Method() override {}
+  void BadBaseMethodNoAttr() {}
+  // CHECK-FIXES: {{^}}  void v_Bad_Base_Method_No_Attr() {}

salman-javed-nz wrote:
> I think this line could spell out more clearly that it is testing the case 
> where the `override` keyword is omitted. I don't think the `NoAttr()` suffix 
> says enough. `override` is not the only Attr.
> 
> Possible solutions:
> - You could incorporate the word "override" in the method name e.g. 
> `BadBaseMethodNoOverride()`, or
> - You could put a `/* override */` where `override` normally would be to 
> bring attention to the fact that it is missing, or
> - You could add a comment explaining what's going on, like the `// Overriding 
> a badly-named base isn't a new violation` a couple of lines above.
> 
> Feel free to do what you think is the least janky.




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp:313
+BadBaseMethodNoAttr();
+// CHECK-FIXES: {{^}}v_Bad_Base_Method_No_Attr();
   }

I would throw in the
```
this->BadBaseMethodNoAttr();
AOverridden::BadBaseMethodNoAttr();
COverriding::BadBaseMethodNoAttr();
```
lines as well.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp:335
+  //(note that there could be specializations of the template base 
class, though)
+  void BadBaseMethod() override{}
+





Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp:365
+  // Overriding a badly-named base isn't a new violation.
+  void BadBaseMethod() override{}
+  // CHECK-FIXES: {{^}}  void v_Bad_Base_Method() override{}





Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp:370
+template class CTIOverriding;
+
 template 

Are you missing another `VirtualCall()` function here? (to test `ATIOverridden` 
and `CTIOverriding`?)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113830/new/

https://reviews.llvm.org/D113830

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fabian Wolff via Phabricator via cfe-commits]

fwolff updated this revision to Diff 387121.
fwolff added a comment.

Thanks for your comments @salman-javed-nz! I have expanded the tests now 
according to your suggestions.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113830/new/

https://reviews.llvm.org/D113830

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

Index: clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
@@ -285,6 +285,10 @@
   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 {
@@ -292,6 +296,8 @@
   // Overriding a badly-named base isn't a new violation.
   void BadBaseMethod() override {}
   // CHECK-FIXES: {{^}}  void v_Bad_Base_Method() override {}
+  void BadBaseMethodNoAttr() {}
+  // CHECK-FIXES: {{^}}  void v_Bad_Base_Method_No_Attr() {}
 
   void foo() {
 BadBaseMethod();
@@ -302,14 +308,66 @@
 // 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();
   }
 };
 
-void VirtualCall(AOverridden _vItem) {
+// Same test as above, now with a dependent base class.
+template
+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
+class CTOverriding : public ATOverridden {
+  // 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
+void VirtualCall(AOverridden _vItem, ATOverridden _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
+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
+class CTIOverriding : public ATIOverridden {
+  // Overriding a badly-named base isn't a new violation.
+  void BadBaseMethod() override{}
+  // CHECK-FIXES: {{^}}  void v_Bad_Base_Method() override{}
+};
+
+template class CTIOverriding;
+
 template 
 class CRTPBase {
 public:
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
@@ -1257,7 +1257,7 @@
 
   if (const auto *Decl = dyn_cast(D)) {
 if (Decl->isMain() || !Decl->isUserProvided() ||
-Decl->size_overridden_methods() > 0)
+Decl->size_overridden_methods() > 0 || Decl->hasAttr())
   return SK_Invalid;
 
 // If this method has the same name as any base method, this is likely
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp:329
+
+  // FIXME: The fixes from ATOverridden should be propagated to the following 
call
+  a_vTitem.BadBaseMethod();

The fixes /do/ seem to be propagated if there is an explicit template 
instantiation.

```lang=cpp
template 
struct A
{
virtual void method() {}
};

template 
struct B : A
{
void method() override {}
void CallVirtual() { this->method(); }
};

template class B;
```

Running with `clang-tidy -fix` and `readability-identifier-naming.FunctionCase 
= CamelCase` gives:
```lang=cpp
template 
struct A
{
virtual void Method() {}
};

template 
struct B : A
{
void Method() override {}
void CallVirtual() { this->Method(); }
};

template class B;
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113830/new/

https://reviews.llvm.org/D113830

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:1260
 if (Decl->isMain() || !Decl->isUserProvided() ||
-Decl->size_overridden_methods() > 0)
+Decl->size_overridden_methods() > 0 || Decl->hasAttr())
   return SK_Invalid;

Adding a call to `hasAttr()` looks OK to me -- this is in line 
with [[ https://clang.llvm.org/doxygen/ASTMatchers_8h_source.html#l06002 | 
AST_MATCHER(CXXMethodDecl, isOverride)]]. Other clang-tidy checks have done the 
same thing.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp:282-294
 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;
 };

How about a test to check that diagnostics are generated even if the `override` 
keyword is omitted.
This challenges the `Decl->size_overridden_methods() > 0` portion of the `if` 
statement.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp:321
+  //(note that there could be specializations of the template base 
class, though)
+  void BadBaseMethod() override{}
+};

What should happen if the base method is overridden without the `override` 
keyword?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113830/new/

https://reviews.llvm.org/D113830

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

Happy to take a look at this, and do some of the initial review legwork, but 
let's leave final approval to @aaron.ballman.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113830/new/

https://reviews.llvm.org/D113830

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fabian Wolff via Phabricator via cfe-commits]

fwolff created this revision.
fwolff added reviewers: alexfh, salman-javed-nz.
fwolff added a project: clang-tools-extra.
Herald added subscribers: carlosgalvezp, xazax.hun.
fwolff requested review of this revision.
Herald added a subscriber: cfe-commits.

Fixes part of PR#45815. 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, which makes a difference for 
dependent base classes.

The other issue mentioned in PR#45815 is not solved by this patch: Applying the 
fix provided by `readability-identifier-naming` only changes the name in the 
base class, not in the derived class(es) or at any call sites. This is 
difficult to fix, because in addition to the template base class, there could 
be specializations of the base class that also contain the overridden method, 
which is why applying the fix from the base class in the derived class in 
general would not lead to correct code.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D113830

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


Index: 
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
@@ -305,9 +305,29 @@
   }
 };
 
-void VirtualCall(AOverridden _vItem) {
+template
+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;
+};
+
+template
+class CTOverriding : public ATOverridden {
+  // 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{}
+};
+
+template
+void VirtualCall(AOverridden _vItem, ATOverridden _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();
 }
 
 template 
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
@@ -1257,7 +1257,7 @@
 
   if (const auto *Decl = dyn_cast(D)) {
 if (Decl->isMain() || !Decl->isUserProvided() ||
-Decl->size_overridden_methods() > 0)
+Decl->size_overridden_methods() > 0 || Decl->hasAttr())
   return SK_Invalid;
 
 // If this method has the same name as any base method, this is likely


Index: clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
@@ -305,9 +305,29 @@
   }
 };
 
-void VirtualCall(AOverridden _vItem) {
+template
+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;
+};
+
+template
+class CTOverriding : public ATOverridden {
+  // 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{}
+};
+
+template
+void VirtualCall(AOverridden _vItem, ATOverridden _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();
 }
 
 template 
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
@@ -1257,7 +1257,7 @@
 
   if (const auto *Decl = dyn_cast(D)) {
 if (Decl->isMain() || !Decl->isUserProvided() ||
-Decl->size_overridden_methods() > 0)
+Decl->size_overridden_methods() > 0 || Decl->hasAttr())
   return SK_Invalid;