[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<OverrideAttr>())
       return SK_Invalid;
----------------
Adding a call to `hasAttr<OverrideAttr>()` 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