[clang-tidy] modified identifier naming case to use CT_AnyCase for ignoring case style

[• jutocz]

Summary

Using CaseType::CT_AnyCase for selected identifier results in inheriting case style setting from more basic identifier type.

This patch changes CT_AnyCase behavior to ignore case style of specified identifier. If case style was not set, llvm::Optional will be used for keeping this information (llvm::Optional<>::hasVal), thus CT_AnyCase will no longer mean more general identifier style should be used.

This eliminates false-positives when naming convention is not clear for specific areas of code (legacy, third party) or for selected types.

[alexfh]

I understand your use case, but this patch makes the check's behavior more confusing: having both "any case" and "ignore case" with subtle differences in behavior seems very misleading. The problem seems to be coming from the usage of CT_AnyCase to denote uninitialized style. Should we just remove NamingStyle::isSet and use llvm::Optional<NamingStyle> instead of NamingStyle where appropriate?

[alexfh]

BTW, next time please add cfe-commits to subscribers when you create the patch to get it sent properly to the mailing list.

[berenm]

In D30931#700690, @alexfh wrote:

I understand your use case, but this patch makes the check's behavior more confusing: having both "any case" and "ignore case" with subtle differences in behavior seems very misleading. The problem seems to be coming from the usage of CT_AnyCase to denote uninitialized style. Should we just remove NamingStyle::isSet and use llvm::Optional<NamingStyle> instead of NamingStyle where appropriate?

I agree

[• jutocz]

You are right, it does look misleading. I'll try to modify it the way you suggest (though I'm new to LLVM, so be ready to give me more comments;).

[alexfh]

In D30931#702578, @jutocz wrote:

You are right, it does look misleading. I'll try to modify it the way you suggest (though I'm new to LLVM, so be ready to give me more comments;)

Sure, thank you for the work!

[• jutocz]

I've modified patch to use llvm::Optional for not defined naming style and naming case.
I had to use llvm::Optional in two places:

• for entries in collection of naming styles (IdentifierNamingCheck::NamingStyles) where style for specific identifier was not set (more general style will be used in this case)
• for case type in NamingStyle, where case style was not set but prefix or postfix was (this remains a bit unintuitive, bacause case style will be ignored and only prefix or postfix will get validated)

[alexfh]

Could you generate a diff with full context (http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface)?

[• jutocz]

Updated diff to show full context

[alexfh]

A couple of nits, otherwise looks good. Do you need me to commit the patch for you?

clang-tidy/readability/IdentifierNamingCheck.cpp

169 ↗

(On Diff #92205)

llvm::None is more idiomatic.

181 ↗

(On Diff #92205)

llvm::None here as well.

211 ↗

(On Diff #92205)

.hasValue() can be omitted, since llvm::Optional<> defines operator bool and operator *, and thus can be used in a similar fashion as a pointer / smart pointer: if (opt) DoSomething(*opt);.

[• jutocz]

I like the idea of using llvm::None plus operator bool and operator *.
I've updated the diff to use llvm::Optional this way .

And yes, I need your help in committing the patch. Thanks.

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

enum CaseType {
     CT_CamelBack,
     CT_UpperCase,
     CT_CamelCase,
     CT_CamelSnakeCase,
     CT_CamelSnakeBack
   };
 
   struct NamingStyle {
-    NamingStyle() : Case(CT_AnyCase) {}
+    NamingStyle() = default;
 
-    NamingStyle(CaseType Case, const std::string &Prefix,
+    NamingStyle(llvm::Optional<CaseType> Case, 
+                const std::string &Prefix,
                 const std::string &Suffix)
         : Case(Case), Prefix(Prefix), Suffix(Suffix) {}
 
-    CaseType Case;
+    llvm::Optional<CaseType> Case;
     std::string Prefix;
     std::string Suffix;
-
-    bool isSet() const {
-      return !(Case == CT_AnyCase && Prefix.empty() && Suffix.empty());
-    }
   };
 
   /// \brief Holds an identifier name check failure, tracking the kind of the
   /// identifer, its possible fixup and the starting locations of all the
   /// identifier usages.
   struct NamingCheckFailure {
     std::string KindName;
     std::string Fixup;
public:
   /// Check Macros for style violations.
   void checkMacro(SourceManager &sourceMgr, const Token &MacroNameTok,
                   const MacroInfo *MI);
 
   /// Add a usage of a macro if it already has a violation.
   void expandMacro(const Token &MacroNameTok, const MacroInfo *MI);
 
 private:
-  std::vector<NamingStyle> NamingStyles;
+  std::vector<llvm::Optional<NamingStyle> > NamingStyles;
   bool IgnoreFailedSplit;
   NamingCheckFailureMap NamingCheckFailures;
 };
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_IDENTIFIERNAMINGCHECK_H

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

private:
   IdentifierNamingCheck *Check;
 };
 } // namespace
 
 IdentifierNamingCheck::IdentifierNamingCheck(StringRef Name,
                                              ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context) {
   auto const fromString = [](StringRef Str) {
-    return llvm::StringSwitch<CaseType>(Str)
+    return llvm::StringSwitch<llvm::Optional<CaseType> >(Str)
+        .Case("aNy_CasE", CT_AnyCase)
         .Case("lower_case", CT_LowerCase)
         .Case("UPPER_CASE", CT_UpperCase)
         .Case("camelBack", CT_CamelBack)
         .Case("CamelCase", CT_CamelCase)
         .Case("Camel_Snake_Case", CT_CamelSnakeCase)
         .Case("camel_Snake_Back", CT_CamelSnakeBack)
-        .Default(CT_AnyCase);
+        .Default(llvm::None);
   };
 
   for (auto const &Name : StyleNames) {
-    NamingStyles.push_back(
-        NamingStyle(fromString(Options.get((Name + "Case").str(), "")),
-                    Options.get((Name + "Prefix").str(), ""),
-                    Options.get((Name + "Suffix").str(), "")));
+    auto const caseOptional =
+        fromString(Options.get((Name + "Case").str(), ""));
+    auto prefix = Options.get((Name + "Prefix").str(), "");
+    auto postfix = Options.get((Name + "Suffix").str(), "");
+
+    if (caseOptional || !prefix.empty() || !postfix.empty()) {
+      NamingStyles.push_back(NamingStyle(caseOptional, prefix, postfix));
+    } else {
+      NamingStyles.push_back(llvm::None);
+    }
   }
 
   IgnoreFailedSplit = Options.get("IgnoreFailedSplit", 0);
 }
 
 void IdentifierNamingCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   auto const toString = [](CaseType Type) {
     switch (Type) {
auto const toString = [](CaseType Type) {
     case CT_CamelSnakeBack:
       return "camel_Snake_Back";
     }
 
     llvm_unreachable("Unknown Case Type");
   };
 
   for (size_t i = 0; i < SK_Count; ++i) {
+    if (NamingStyles[i]) {
+      if (NamingStyles[i]->Case) {
         Options.store(Opts, (StyleNames[i] + "Case").str(),
-                  toString(NamingStyles[i].Case));
+                      toString(*NamingStyles[i]->Case));
+      }
       Options.store(Opts, (StyleNames[i] + "Prefix").str(),
-                  NamingStyles[i].Prefix);
+                    NamingStyles[i]->Prefix);
       Options.store(Opts, (StyleNames[i] + "Suffix").str(),
-                  NamingStyles[i].Suffix);
+                    NamingStyles[i]->Suffix);
+    }
   }
 
   Options.store(Opts, "IgnoreFailedSplit", IgnoreFailedSplit);
 }
 
 void IdentifierNamingCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(namedDecl().bind("decl"), this);
   Finder->addMatcher(usingDecl().bind("using"), this);
static bool matchesStyle(StringRef Name,
   else
     Matches = false;
 
   if (Name.endswith(Style.Suffix))
     Name = Name.drop_back(Style.Suffix.size());
   else
     Matches = false;
 
-  if (!Matchers[static_cast<size_t>(Style.Case)].match(Name))
+  if (Style.Case &&
+      !Matchers[static_cast<size_t>(*Style.Case)].match(Name))
     Matches = false;
 
   return Matches;
 }
 
 static std::string fixupWithCase(StringRef Name,
                                  IdentifierNamingCheck::CaseType Case) {
   static llvm::Regex Splitter(
case IdentifierNamingCheck::CT_CamelSnakeBack:
     }
     break;
   }
 
   return Fixup;
 }
 
 static std::string fixupWithStyle(StringRef Name,
-                                  IdentifierNamingCheck::NamingStyle Style) {
-  return Style.Prefix + fixupWithCase(Name, Style.Case) + Style.Suffix;
+                            const IdentifierNamingCheck::NamingStyle &Style) {
+  return Style.Prefix +
+         fixupWithCase(Name, Style.Case.getValueOr(
+                                 IdentifierNamingCheck::CaseType::CT_AnyCase)) +
+         Style.Suffix;
 }
 
 static StyleKind findStyleKind(
     const NamedDecl *D,
-    const std::vector<IdentifierNamingCheck::NamingStyle> &NamingStyles) {
-  if (isa<TypedefDecl>(D) && NamingStyles[SK_Typedef].isSet())
+    const std::vector<llvm::Optional<IdentifierNamingCheck::NamingStyle>>
+        &NamingStyles) {
+  if (isa<TypedefDecl>(D) && NamingStyles[SK_Typedef])
     return SK_Typedef;
 
-  if (isa<TypeAliasDecl>(D) && NamingStyles[SK_TypeAlias].isSet())
+  if (isa<TypeAliasDecl>(D) && NamingStyles[SK_TypeAlias])
     return SK_TypeAlias;
 
   if (const auto *Decl = dyn_cast<NamespaceDecl>(D)) {
     if (Decl->isAnonymousNamespace())
       return SK_Invalid;
 
-    if (Decl->isInline() && NamingStyles[SK_InlineNamespace].isSet())
+    if (Decl->isInline() && NamingStyles[SK_InlineNamespace])
       return SK_InlineNamespace;
 
-    if (NamingStyles[SK_Namespace].isSet())
+    if (NamingStyles[SK_Namespace])
       return SK_Namespace;
   }
 
-  if (isa<EnumDecl>(D) && NamingStyles[SK_Enum].isSet())
+  if (isa<EnumDecl>(D) && NamingStyles[SK_Enum])
     return SK_Enum;
 
   if (isa<EnumConstantDecl>(D)) {
-    if (NamingStyles[SK_EnumConstant].isSet())
+    if (NamingStyles[SK_EnumConstant])
       return SK_EnumConstant;
 
-    if (NamingStyles[SK_Constant].isSet())
+    if (NamingStyles[SK_Constant])
       return SK_Constant;
 
     return SK_Invalid;
   }
 
   if (const auto *Decl = dyn_cast<CXXRecordDecl>(D)) {
     if (Decl->isAnonymousStructOrUnion())
       return SK_Invalid;
 
     if (!Decl->getCanonicalDecl()->isThisDeclarationADefinition())
       return SK_Invalid;
 
     if (Decl->hasDefinition() && Decl->isAbstract() &&
-        NamingStyles[SK_AbstractClass].isSet())
+        NamingStyles[SK_AbstractClass])
       return SK_AbstractClass;
 
-    if (Decl->isStruct() && NamingStyles[SK_Struct].isSet())
+    if (Decl->isStruct() && NamingStyles[SK_Struct])
       return SK_Struct;
 
-    if (Decl->isStruct() && NamingStyles[SK_Class].isSet())
+    if (Decl->isStruct() && NamingStyles[SK_Class])
       return SK_Class;
 
-    if (Decl->isClass() && NamingStyles[SK_Class].isSet())
+    if (Decl->isClass() && NamingStyles[SK_Class])
       return SK_Class;
 
-    if (Decl->isClass() && NamingStyles[SK_Struct].isSet())
+    if (Decl->isClass() && NamingStyles[SK_Struct])
       return SK_Struct;
 
-    if (Decl->isUnion() && NamingStyles[SK_Union].isSet())
+    if (Decl->isUnion() && NamingStyles[SK_Union])
       return SK_Union;
 
-    if (Decl->isEnum() && NamingStyles[SK_Enum].isSet())
+    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].isSet())
+        NamingStyles[SK_ConstantMember])
       return SK_ConstantMember;
 
     if (!Type.isNull() && Type.isLocalConstQualified() &&
-        NamingStyles[SK_Constant].isSet())
+        NamingStyles[SK_Constant])
       return SK_Constant;
 
     if (Decl->getAccess() == AS_private &&
-        NamingStyles[SK_PrivateMember].isSet())
+        NamingStyles[SK_PrivateMember])
       return SK_PrivateMember;
 
     if (Decl->getAccess() == AS_protected &&
-        NamingStyles[SK_ProtectedMember].isSet())
+        NamingStyles[SK_ProtectedMember])
       return SK_ProtectedMember;
 
-    if (Decl->getAccess() == AS_public && NamingStyles[SK_PublicMember].isSet())
+    if (Decl->getAccess() == AS_public &&
+        NamingStyles[SK_PublicMember])
       return SK_PublicMember;
 
-    if (NamingStyles[SK_Member].isSet())
+    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].isSet())
+    if (Decl->isConstexpr() && NamingStyles[SK_ConstexprVariable])
       return SK_ConstexprVariable;
 
     if (!Type.isNull() && Type.isLocalConstQualified() &&
-        NamingStyles[SK_ConstantParameter].isSet())
+        NamingStyles[SK_ConstantParameter])
       return SK_ConstantParameter;
 
     if (!Type.isNull() && Type.isLocalConstQualified() &&
-        NamingStyles[SK_Constant].isSet())
+        NamingStyles[SK_Constant])
       return SK_Constant;
 
-    if (Decl->isParameterPack() && NamingStyles[SK_ParameterPack].isSet())
+    if (Decl->isParameterPack() && NamingStyles[SK_ParameterPack])
       return SK_ParameterPack;
 
-    if (NamingStyles[SK_Parameter].isSet())
+    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].isSet())
+    if (Decl->isConstexpr() && NamingStyles[SK_ConstexprVariable])
       return SK_ConstexprVariable;
 
     if (!Type.isNull() && Type.isLocalConstQualified() &&
-        Decl->isStaticDataMember() && NamingStyles[SK_ClassConstant].isSet())
+        Decl->isStaticDataMember() && NamingStyles[SK_ClassConstant])
       return SK_ClassConstant;
 
     if (!Type.isNull() && Type.isLocalConstQualified() &&
-        Decl->isFileVarDecl() && NamingStyles[SK_GlobalConstant].isSet())
+        Decl->isFileVarDecl() && NamingStyles[SK_GlobalConstant])
       return SK_GlobalConstant;
 
     if (!Type.isNull() && Type.isLocalConstQualified() &&
-        Decl->isStaticLocal() && NamingStyles[SK_StaticConstant].isSet())
+        Decl->isStaticLocal() && NamingStyles[SK_StaticConstant])
       return SK_StaticConstant;
 
     if (!Type.isNull() && Type.isLocalConstQualified() &&
-        Decl->isLocalVarDecl() && NamingStyles[SK_LocalConstant].isSet())
+        Decl->isLocalVarDecl() && NamingStyles[SK_LocalConstant])
       return SK_LocalConstant;
 
     if (!Type.isNull() && Type.isLocalConstQualified() &&
         Decl->isFunctionOrMethodVarDecl() &&
-        NamingStyles[SK_LocalConstant].isSet())
+        NamingStyles[SK_LocalConstant])
       return SK_LocalConstant;
 
     if (!Type.isNull() && Type.isLocalConstQualified() &&
-        NamingStyles[SK_Constant].isSet())
+        NamingStyles[SK_Constant])
       return SK_Constant;
 
-    if (Decl->isStaticDataMember() && NamingStyles[SK_ClassMember].isSet())
+    if (Decl->isStaticDataMember() && NamingStyles[SK_ClassMember])
       return SK_ClassMember;
 
-    if (Decl->isFileVarDecl() && NamingStyles[SK_GlobalVariable].isSet())
+    if (Decl->isFileVarDecl() && NamingStyles[SK_GlobalVariable])
       return SK_GlobalVariable;
 
-    if (Decl->isStaticLocal() && NamingStyles[SK_StaticVariable].isSet())
+    if (Decl->isStaticLocal() && NamingStyles[SK_StaticVariable])
       return SK_StaticVariable;
 
-    if (Decl->isLocalVarDecl() && NamingStyles[SK_LocalVariable].isSet())
+    if (Decl->isLocalVarDecl() && NamingStyles[SK_LocalVariable])
       return SK_LocalVariable;
 
     if (Decl->isFunctionOrMethodVarDecl() &&
-        NamingStyles[SK_LocalVariable].isSet())
+        NamingStyles[SK_LocalVariable])
       return SK_LocalVariable;
 
-    if (NamingStyles[SK_Variable].isSet())
+    if (NamingStyles[SK_Variable])
       return SK_Variable;
 
     return SK_Invalid;
   }
 
   if (const auto *Decl = dyn_cast<CXXMethodDecl>(D)) {
     if (Decl->isMain() || !Decl->isUserProvided() ||
         Decl->isUsualDeallocationFunction() ||
         Decl->isCopyAssignmentOperator() || Decl->isMoveAssignmentOperator() ||
         Decl->size_overridden_methods() > 0)
       return SK_Invalid;
 
-    if (Decl->isConstexpr() && NamingStyles[SK_ConstexprMethod].isSet())
+    if (Decl->isConstexpr() && NamingStyles[SK_ConstexprMethod])
       return SK_ConstexprMethod;
 
-    if (Decl->isConstexpr() && NamingStyles[SK_ConstexprFunction].isSet())
+    if (Decl->isConstexpr() && NamingStyles[SK_ConstexprFunction])
       return SK_ConstexprFunction;
 
-    if (Decl->isStatic() && NamingStyles[SK_ClassMethod].isSet())
+    if (Decl->isStatic() && NamingStyles[SK_ClassMethod])
       return SK_ClassMethod;
 
-    if (Decl->isVirtual() && NamingStyles[SK_VirtualMethod].isSet())
+    if (Decl->isVirtual() && NamingStyles[SK_VirtualMethod])
       return SK_VirtualMethod;
 
     if (Decl->getAccess() == AS_private &&
-        NamingStyles[SK_PrivateMethod].isSet())
+        NamingStyles[SK_PrivateMethod])
       return SK_PrivateMethod;
 
     if (Decl->getAccess() == AS_protected &&
-        NamingStyles[SK_ProtectedMethod].isSet())
+        NamingStyles[SK_ProtectedMethod])
       return SK_ProtectedMethod;
 
-    if (Decl->getAccess() == AS_public && NamingStyles[SK_PublicMethod].isSet())
+    if (Decl->getAccess() == AS_public &&
+        NamingStyles[SK_PublicMethod])
       return SK_PublicMethod;
 
-    if (NamingStyles[SK_Method].isSet())
+    if (NamingStyles[SK_Method])
       return SK_Method;
 
-    if (NamingStyles[SK_Function].isSet())
+    if (NamingStyles[SK_Function])
       return SK_Function;
 
     return SK_Invalid;
   }
 
   if (const auto *Decl = dyn_cast<FunctionDecl>(D)) {
     if (Decl->isMain())
       return SK_Invalid;
 
-    if (Decl->isConstexpr() && NamingStyles[SK_ConstexprFunction].isSet())
+    if (Decl->isConstexpr() && NamingStyles[SK_ConstexprFunction])
       return SK_ConstexprFunction;
 
-    if (Decl->isGlobal() && NamingStyles[SK_GlobalFunction].isSet())
+    if (Decl->isGlobal() && NamingStyles[SK_GlobalFunction])
       return SK_GlobalFunction;
 
-    if (NamingStyles[SK_Function].isSet())
+    if (NamingStyles[SK_Function])
       return SK_Function;
   }
 
   if (isa<TemplateTypeParmDecl>(D)) {
-    if (NamingStyles[SK_TypeTemplateParameter].isSet())
+    if (NamingStyles[SK_TypeTemplateParameter])
       return SK_TypeTemplateParameter;
 
-    if (NamingStyles[SK_TemplateParameter].isSet())
+    if (NamingStyles[SK_TemplateParameter])
       return SK_TemplateParameter;
 
     return SK_Invalid;
   }
 
   if (isa<NonTypeTemplateParmDecl>(D)) {
-    if (NamingStyles[SK_ValueTemplateParameter].isSet())
+    if (NamingStyles[SK_ValueTemplateParameter])
       return SK_ValueTemplateParameter;
 
-    if (NamingStyles[SK_TemplateParameter].isSet())
+    if (NamingStyles[SK_TemplateParameter])
       return SK_TemplateParameter;
 
     return SK_Invalid;
   }
 
   if (isa<TemplateTemplateParmDecl>(D)) {
-    if (NamingStyles[SK_TemplateTemplateParameter].isSet())
+    if (NamingStyles[SK_TemplateTemplateParameter])
       return SK_TemplateTemplateParameter;
 
-    if (NamingStyles[SK_TemplateParameter].isSet())
+    if (NamingStyles[SK_TemplateParameter])
       return SK_TemplateParameter;
 
     return SK_Invalid;
   }
 
   return SK_Invalid;
 }
 
if (const auto *Decl = Result.Nodes.getNodeAs<NamedDecl>("decl")) {
     // replacements with CXXRecordDecl
     if (isa<ClassTemplateSpecializationDecl>(Decl))
       return;
 
     StyleKind SK = findStyleKind(Decl, NamingStyles);
     if (SK == SK_Invalid)
       return;
 
-    NamingStyle Style = NamingStyles[SK];
+    if (!NamingStyles[SK])
+      return;
+
+    const NamingStyle &Style = *NamingStyles[SK];
     StringRef Name = Decl->getName();
     if (matchesStyle(Name, Style))
       return;
 
     std::string KindName = fixupWithCase(StyleNames[SK], CT_LowerCase);
     std::replace(KindName.begin(), KindName.end(), '_', ' ');
 
     std::string Fixup = fixupWithStyle(Name, Style);
if (StringRef(Fixup).equals(Name)) {
       addUsage(NamingCheckFailures, Decl, Range);
     }
   }
 }
 
 void IdentifierNamingCheck::checkMacro(SourceManager &SourceMgr,
                                        const Token &MacroNameTok,
                                        const MacroInfo *MI) {
+  if (!NamingStyles[SK_MacroDefinition])
+    return;
+
   StringRef Name = MacroNameTok.getIdentifierInfo()->getName();
-  NamingStyle Style = NamingStyles[SK_MacroDefinition];
+  const NamingStyle &Style = *NamingStyles[SK_MacroDefinition];
   if (matchesStyle(Name, Style))
     return;
 
   std::string KindName =
       fixupWithCase(StyleNames[SK_MacroDefinition], CT_LowerCase);
   std::replace(KindName.begin(), KindName.end(), '_', ' ');
 
   std::string Fixup = fixupWithStyle(Name, Style);