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