[clang-tidy] Correctly classify constant arrays and constant strings as constants when checking identifiers naming

[berenm]

Summary

They are not locally const qualified so they weren't classified as
constants by the readability-identifier-naming check.

[alexfh]

Sorry for the delay. I missed this revision somehow. Please add cfe-commits to the subscribers list so that others can chime in.

[alexfh]

One nit. Otherwise seems fine.

clang-tidy/readability/IdentifierNamingCheck.cpp

502 ↗

(On Diff #120546)

Can we check for !Type.isNull() && Type.isConstQualified() once here?

if (!Type.isNull() && Type.isConstQualified()) {
  if (Decl->isStaticDataMember() && NamingStyles[SK_ClassConstant])
    return SK_ClassConstant;
  if (Decl->isFileVarDecl() && NamingStyles[SK_GlobalConstant])
    return SK_GlobalConstant;
  ...
}

[berenm]

Factor !Type.isNull() && Type.isConstQualified() condition

[berenm]

In D39363#948134, @alexfh wrote:

Sorry for the delay. I missed this revision somehow. Please add cfe-commits to the subscribers list so that others can chime in.

No worries, I'll add it in the future.

Thanks!

[alexfh]

Looks good!

Thank you for the fix! Do you need someone to commit it for you? If yes, please rebase the fix on top of the current HEAD and let me know.

[berenm]

Rebase patch on upstream HEAD

[berenm]

Yes, please could you commit it for me? Thanks!

clang-tools-extra/trunk/clang-tidy/readability/IdentifierNamingCheck.cpp

if (Decl->isEnum() && NamingStyles[SK_Enum])
       return SK_Enum;
 
     return SK_Invalid;
   }
 
   if (const auto *Decl = dyn_cast<FieldDecl>(D)) {
     QualType Type = Decl->getType();
 
-    if (!Type.isNull() && Type.isLocalConstQualified() &&
-        NamingStyles[SK_ConstantMember])
+    if (!Type.isNull() && Type.isConstQualified()) {
+      if (NamingStyles[SK_ConstantMember])
         return SK_ConstantMember;
 
-    if (!Type.isNull() && Type.isLocalConstQualified() &&
-        NamingStyles[SK_Constant])
+      if (NamingStyles[SK_Constant])
         return SK_Constant;
+    }
 
     if (Decl->getAccess() == AS_private && NamingStyles[SK_PrivateMember])
       return SK_PrivateMember;
 
     if (Decl->getAccess() == AS_protected && NamingStyles[SK_ProtectedMember])
       return SK_ProtectedMember;
 
     if (Decl->getAccess() == AS_public && NamingStyles[SK_PublicMember])
       return SK_PublicMember;
 
     if (NamingStyles[SK_Member])
       return SK_Member;
 
     return SK_Invalid;
   }
 
   if (const auto *Decl = dyn_cast<ParmVarDecl>(D)) {
     QualType Type = Decl->getType();
 
     if (Decl->isConstexpr() && NamingStyles[SK_ConstexprVariable])
       return SK_ConstexprVariable;
 
-    if (!Type.isNull() && Type.isLocalConstQualified() &&
-        NamingStyles[SK_ConstantParameter])
+    if (!Type.isNull() && Type.isConstQualified()) {
+      if (NamingStyles[SK_ConstantParameter])
         return SK_ConstantParameter;
 
-    if (!Type.isNull() && Type.isLocalConstQualified() &&
-        NamingStyles[SK_Constant])
+      if (NamingStyles[SK_Constant])
         return SK_Constant;
+    }
 
     if (Decl->isParameterPack() && NamingStyles[SK_ParameterPack])
       return SK_ParameterPack;
 
     if (NamingStyles[SK_Parameter])
       return SK_Parameter;
 
     return SK_Invalid;
   }
 
   if (const auto *Decl = dyn_cast<VarDecl>(D)) {
     QualType Type = Decl->getType();
 
     if (Decl->isConstexpr() && NamingStyles[SK_ConstexprVariable])
       return SK_ConstexprVariable;
 
-    if (!Type.isNull() && Type.isLocalConstQualified() &&
-        Decl->isStaticDataMember() && NamingStyles[SK_ClassConstant])
+    if (!Type.isNull() && Type.isConstQualified()) {
+      if (Decl->isStaticDataMember() && NamingStyles[SK_ClassConstant])
         return SK_ClassConstant;
 
-    if (!Type.isNull() && Type.isLocalConstQualified() &&
-        Decl->isFileVarDecl() && NamingStyles[SK_GlobalConstant])
+      if (Decl->isFileVarDecl() && NamingStyles[SK_GlobalConstant])
         return SK_GlobalConstant;
 
-    if (!Type.isNull() && Type.isLocalConstQualified() &&
-        Decl->isStaticLocal() && NamingStyles[SK_StaticConstant])
+      if (Decl->isStaticLocal() && NamingStyles[SK_StaticConstant])
         return SK_StaticConstant;
 
-    if (!Type.isNull() && Type.isLocalConstQualified() &&
-        Decl->isLocalVarDecl() && NamingStyles[SK_LocalConstant])
+      if (Decl->isLocalVarDecl() && NamingStyles[SK_LocalConstant])
         return SK_LocalConstant;
 
-    if (!Type.isNull() && Type.isLocalConstQualified() &&
-        Decl->isFunctionOrMethodVarDecl() && NamingStyles[SK_LocalConstant])
+      if (Decl->isFunctionOrMethodVarDecl() && NamingStyles[SK_LocalConstant])
         return SK_LocalConstant;
 
-    if (!Type.isNull() && Type.isLocalConstQualified() &&
-        NamingStyles[SK_Constant])
+      if (NamingStyles[SK_Constant])
         return SK_Constant;
+    }
 
     if (Decl->isStaticDataMember() && NamingStyles[SK_ClassMember])
       return SK_ClassMember;
 
     if (Decl->isFileVarDecl() && NamingStyles[SK_GlobalVariable])
       return SK_GlobalVariable;
 
     if (Decl->isStaticLocal() && NamingStyles[SK_StaticVariable])
bool RangeCanBeFixed =
       RangeIsEntirelyWithinMacroArgument || !RangeContainsMacroExpansion;
   Failure.ShouldFix = RangeCanBeFixed;
 }
 
 /// Convenience method when the usage to be added is a NamedDecl
 static void addUsage(IdentifierNamingCheck::NamingCheckFailureMap &Failures,
                      const NamedDecl *Decl, SourceRange Range,
                      SourceManager *SourceMgr = nullptr) {
-  return addUsage(Failures, IdentifierNamingCheck::NamingCheckId(
-                                Decl->getLocation(), Decl->getNameAsString()),
+  return addUsage(Failures,
+                  IdentifierNamingCheck::NamingCheckId(Decl->getLocation(),
+                                                       Decl->getNameAsString()),
                   Range, SourceMgr);
 }
 
 void IdentifierNamingCheck::check(const MatchFinder::MatchResult &Result) {
   if (const auto *Decl =
           Result.Nodes.getNodeAs<CXXConstructorDecl>("classRef")) {
     if (Decl->isImplicit())
       return;
 
     addUsage(NamingCheckFailures, Decl->getParent(),
              Decl->getNameInfo().getSourceRange());
 
     for (const auto *Init : Decl->inits()) {
       if (!Init->isWritten() || Init->isInClassMemberInitializer())
         continue;
       if (const auto *FD = Init->getAnyMember())
-        addUsage(NamingCheckFailures, FD, SourceRange(Init->getMemberLocation()));
+        addUsage(NamingCheckFailures, FD,
+                 SourceRange(Init->getMemberLocation()));
       // Note: delegating constructors and base class initializers are handled
       // via the "typeLoc" matcher.
     }
     return;
   }
 
   if (const auto *Decl =
           Result.Nodes.getNodeAs<CXXDestructorDecl>("classRef")) {

clang-tools-extra/trunk/test/clang-tidy/readability-identifier-naming.cpp

// CHECK-FIXES: {{^}}static void staticFunction() {{{$}}
 
   ::FOO_NS::InlineNamespace::abstract_class::CLASS_METHOD();
 // CHECK-FIXES: {{^}}  ::foo_ns::inline_namespace::AAbstractClass::classMethod();{{$}}
   ::FOO_NS::InlineNamespace::static_Function();
 // CHECK-FIXES: {{^}}  ::foo_ns::inline_namespace::staticFunction();{{$}}
 
   using ::FOO_NS::InlineNamespace::CE_function;
 // CHECK-FIXES: {{^}}  using ::foo_ns::inline_namespace::ce_function;{{$}}
+
+  unsigned MY_LOCAL_array[] = {1,2,3};
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: invalid case style for local variable 'MY_LOCAL_array'
+// CHECK-FIXES: {{^}}  unsigned my_local_array[] = {1,2,3};{{$}}
+
+  unsigned const MyConstLocal_array[] = {1,2,3};
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: invalid case style for local constant 'MyConstLocal_array'
+// CHECK-FIXES: {{^}}  unsigned const kMyConstLocalArray[] = {1,2,3};{{$}}
+
+  static unsigned MY_STATIC_array[] = {1,2,3};
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: invalid case style for static variable 'MY_STATIC_array'
+// CHECK-FIXES: {{^}}  static unsigned s_myStaticArray[] = {1,2,3};{{$}}
+
+  static unsigned const MyConstStatic_array[] = {1,2,3};
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: invalid case style for static constant 'MyConstStatic_array'
+// CHECK-FIXES: {{^}}  static unsigned const MY_CONST_STATIC_ARRAY[] = {1,2,3};{{$}}
+
+  char MY_LOCAL_string[] = "123";
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for local variable 'MY_LOCAL_string'
+// CHECK-FIXES: {{^}}  char my_local_string[] = "123";{{$}}
+
+  char const MyConstLocal_string[] = "123";
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: invalid case style for local constant 'MyConstLocal_string'
+// CHECK-FIXES: {{^}}  char const kMyConstLocalString[] = "123";{{$}}
+
+  static char MY_STATIC_string[] = "123";
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: invalid case style for static variable 'MY_STATIC_string'
+// CHECK-FIXES: {{^}}  static char s_myStaticString[] = "123";{{$}}
+
+  static char const MyConstStatic_string[] = "123";
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: invalid case style for static constant 'MyConstStatic_string'
+// CHECK-FIXES: {{^}}  static char const MY_CONST_STATIC_STRING[] = "123";{{$}}
 }
 
 #define MY_TEST_Macro(X) X()
 // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: invalid case style for macro definition 'MY_TEST_Macro'
 // CHECK-FIXES: {{^}}#define MY_TEST_MACRO(X) X()
 
 void MY_TEST_Macro(function) {}
 // CHECK-FIXES: {{^}}void MY_TEST_MACRO(function) {}
 }
 }
 
 template <typename t_t> struct a {
   typename t_t::template b<> c;
+
+  char const MY_ConstMember_string[4] = "123";
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: invalid case style for constant member 'MY_ConstMember_string'
+// CHECK-FIXES: {{^}}  char const my_const_member_string[4] = "123";{{$}}
+
+  static char const MyConstClass_string[];
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: invalid case style for class constant 'MyConstClass_string'
+// CHECK-FIXES: {{^}}  static char const kMyConstClassString[];{{$}}
 };
 
+template<typename t_t>
+char const a<t_t>::MyConstClass_string[] = "123";
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: invalid case style for class constant 'MyConstClass_string'
+// CHECK-FIXES: {{^}}char const a<t_t>::kMyConstClassString[] = "123";{{$}}
+
 template <template <typename> class A> struct b { A<int> c; };
+
+unsigned MY_GLOBAL_array[] = {1,2,3};
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: invalid case style for global variable 'MY_GLOBAL_array'
+// CHECK-FIXES: {{^}}unsigned g_my_global_array[] = {1,2,3};{{$}}
+
+unsigned const MyConstGlobal_array[] = {1,2,3};
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for global constant 'MyConstGlobal_array'
+// CHECK-FIXES: {{^}}unsigned const MY_CONST_GLOBAL_ARRAY[] = {1,2,3};{{$}}