[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

llvm::None is more idiomatic.

184

llvm::None here as well.

212

.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-tidy/readability/IdentifierNamingCheck.h

public:
 
   enum CaseType {
     CT_AnyCase = 0,
     CT_LowerCase,
     CT_CamelBack,
     CT_UpperCase,
     CT_CamelCase,
     CT_CamelSnakeCase,
-    CT_CamelSnakeBack
+    CT_CamelSnakeBack,
+    CT_IgnoreCase
   };
 
   struct NamingStyle {
     NamingStyle() : Case(CT_AnyCase) {}
 
     NamingStyle(CaseType Case, const std::string &Prefix,
                 const std::string &Suffix)
         : Case(Case), Prefix(Prefix), Suffix(Suffix) {}

clang-tidy/readability/IdentifierNamingCheck.cpp

IdentifierNamingCheck::IdentifierNamingCheck(StringRef Name,
   auto const fromString = [](StringRef Str) {
     return llvm::StringSwitch<CaseType>(Str)
         .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)
+        .Case("IgnoreCase", CT_IgnoreCase)
         .Default(CT_AnyCase);
   };
 
   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(), "")));
   }
 
   IgnoreFailedSplit = Options.get("IgnoreFailedSplit", 0);
 }
 
 void IdentifierNamingCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   auto const toString = [](CaseType Type) {
     switch (Type) {
     case CT_AnyCase:
       return "aNy_CasE";
     case CT_LowerCase:
       return "lower_case";
     case CT_CamelBack:
       return "camelBack";
     case CT_UpperCase:
       return "UPPER_CASE";
     case CT_CamelCase:
       return "CamelCase";
     case CT_CamelSnakeCase:
       return "Camel_Snake_Case";
     case CT_CamelSnakeBack:
       return "camel_Snake_Back";
+    case CT_IgnoreCase:
+      return "IgnoreCase";
     }
 
     llvm_unreachable("Unknown Case Type");
   };
 
   for (size_t i = 0; i < SK_Count; ++i) {
     Options.store(Opts, (StyleNames[i] + "Case").str(),
                   toString(NamingStyles[i].Case));
     Options.store(Opts, (StyleNames[i] + "Prefix").str(),
                   NamingStyles[i].Prefix);
     Options.store(Opts, (StyleNames[i] + "Suffix").str(),
                   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,
   static llvm::Regex Matchers[] = {
       llvm::Regex("^.*$"),
       llvm::Regex("^[a-z][a-z0-9_]*$"),
       llvm::Regex("^[a-z][a-zA-Z0-9]*$"),
       llvm::Regex("^[A-Z][A-Z0-9_]*$"),
       llvm::Regex("^[A-Z][a-zA-Z0-9]*$"),
       llvm::Regex("^[A-Z]([a-z0-9]*(_[A-Z])?)*"),
       llvm::Regex("^[a-z]([a-z0-9]*(_[A-Z])?)*"),
+      llvm::Regex("^.*$"),
   };
 
   bool Matches = true;
   if (Name.startswith(Style.Prefix))
     Name = Name.drop_front(Style.Prefix.size());
   else
     Matches = false;
 
static std::string fixupWithCase(StringRef Name,
   }
 
   if (Words.empty())
     return Name;
 
   std::string Fixup;
   switch (Case) {
   case IdentifierNamingCheck::CT_AnyCase:
+  case IdentifierNamingCheck::CT_IgnoreCase:
     Fixup += Name;
     break;
 
   case IdentifierNamingCheck::CT_LowerCase:
     for (auto const &Word : Words) {
       if (&Word != &Words.front())
         Fixup += "_";
       Fixup += Word.lower();