[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<int>;
+
 template <typename derived_t>
----------------
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