[clang-tidy] Code factorization and cleanup in IdentifierNamingCheck

[berenm]

Summary

This is to level the ground a little bit, in preparation for the changes in http://reviews.llvm.org/D13081.

Code factorization replaces all insertions to NamingCheckFailures map with a unique addUsage function that does the job.
There is also no more difference between the declaration and the references to a given identifier, both cases are treated as ranges in the Usage vector. There is also a check to avoid duplicated ranges to be inserted, which sometimes triggered erroneous replacements.

References can now also be added before the declaration of the identifier is actually found; this looks to be the case for example when a templated class uses its parameters to specialize its templated base class.

[berenm]

clang-tidy/readability/IdentifierNamingCheck.cpp

508 ↗

(On Diff #35440)

Hopefully the number of ranges in Usages will stay low. If not, Usages should be an hash table instead, but it looks like SourceRange isn't hashable as is.

[alexfh]

Please add more context to the diffs.

clang-tidy/readability/IdentifierNamingCheck.cpp

508 ↗

(On Diff #35440)

If the ranges represent unique variable usages, then there definitely will be corner cases that will make this code run sloooowly. I'd prefer this to be a llvm::SmallSet<>, llvm::DenseSet<>, std::unordered_set, std::set, std::multimap<SourceLocation, SourceLocation> or anything else providing a sub-linear lookup time. I'd probably go with llvm::SmallSet<> with a custom comparer for SourceRange.

[berenm]

Changed Usages to RawUsageLocs, as DenseSet storing the raw encoding of SourceLocation instead of a vector or SourceRange.

[berenm]

Reformatting with clang-format

[alexfh]

Looks good with a few comments.

Please tell me, if you need me to commit the patch for you after you address the comments.

clang-tidy/readability/IdentifierNamingCheck.cpp

574 ↗

(On Diff #35669)

This change assumes that the name is always represented by a single token. Please add a comment explaining why this can be done.

clang-tidy/readability/IdentifierNamingCheck.h

65 ↗

(On Diff #35669)

Please add a comment for this struct. Especially for the RawUsageLocs field.

73 ↗

(On Diff #35669)

80 character limit.

[berenm]

Added comments and reformated NamingCheckIdentifer.h with clang-format.

[berenm]

In D13079#253512, @alexfh wrote:

Please tell me, if you need me to commit the patch for you after you address the comments.

Yes please, I cannot commit it myself.

[alexfh]

clang-tidy/readability/IdentifierNamingCheck.cpp

577–578 ↗

(On Diff #35818)

I'd like the comment to cover destructors, operator names and other cases where the actual name consists of multiple tokens (which are probably filtered out somewhere, as we don't need to fix naming for these).

Fine for a follow-up.

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

struct NamingStyle {
     std::string Prefix;
     std::string Suffix;
 
     bool isSet() const {
       return !(Case == CT_AnyCase && Prefix.empty() && Suffix.empty());
     }
   };
 
-private:
-  std::vector<NamingStyle> NamingStyles;
-  bool IgnoreFailedSplit;
-
+  /// \brief Holds an identifier name check failure, tracking the kind of the
+  /// identifer, its possible fixup and the starting locations of all the
+  /// idenfiier usages.
   struct NamingCheckFailure {
     std::string KindName;
     std::string Fixup;
+
+    /// \brief Whether the failure should be fixed or not.
+    ///
+    /// ie: if the identifier was used or declared within a macro we won't offer
+    /// a fixup for safety reasons.
     bool ShouldFix;
-    std::vector<SourceRange> Usages;
+
+    /// \brief A set of all the identifier usages starting SourceLocation, in
+    /// their encoded form.
+    llvm::DenseSet<unsigned> RawUsageLocs;
 
     NamingCheckFailure() : ShouldFix(true) {}
   };
+  typedef llvm::DenseMap<const NamedDecl *, NamingCheckFailure>
+      NamingCheckFailureMap;
 
-  llvm::DenseMap<const NamedDecl *, NamingCheckFailure> NamingCheckFailures;
+private:
+  std::vector<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

 #define DEBUG_TYPE "clang-tidy"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
+// clang-format off
 #define NAMING_KEYS(m) \
     m(Namespace) \
     m(InlineNamespace) \
     m(EnumConstant) \
     m(ConstexprVariable) \
     m(ConstantMember) \
     m(PrivateMember) \
     m(ProtectedMember) \

 
 static StringRef const StyleNames[] = {
 #define STRINGIZE(v) #v,
   NAMING_KEYS(STRINGIZE)
 #undef STRINGIZE
 };
 
 #undef NAMING_KEYS
+// clang-format on
 
 IdentifierNamingCheck::IdentifierNamingCheck(StringRef Name,
                                              ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context) {
   auto const fromString = [](StringRef Str) {
     return llvm::StringSwitch<CaseType>(Str)
         .Case("lower_case", CT_LowerCase)
         .Case("UPPER_CASE", CT_UpperCase)
for (size_t i = 0; i < SK_Count; ++i) {
     Options.store(Opts, (StyleNames[i] + "Suffix").str(),
                   NamingStyles[i].Suffix);
   }
 
   Options.store(Opts, "IgnoreFailedSplit", IgnoreFailedSplit);
 }
 
 void IdentifierNamingCheck::registerMatchers(MatchFinder *Finder) {
-// FIXME: For now, only Decl and DeclRefExpr nodes are visited for checking and
-// replacement. There is a lot of missing cases, such as references to a class
-// name (as in 'const int CMyClass::kClassConstant = 4;'), to an enclosing
-// context (namespace, class, etc).
+  // FIXME: For now, only Decl and DeclRefExpr nodes are visited for checking
+  // and replacement. There is a lot of missing cases, such as references to a
+  // class name (as in 'const int CMyClass::kClassConstant = 4;'), to an
+  // enclosing context (namespace, class, etc).
 
   Finder->addMatcher(namedDecl().bind("decl"), this);
   Finder->addMatcher(declRefExpr().bind("declref"), this);
 }
 
 static bool matchesStyle(StringRef Name,
                          IdentifierNamingCheck::NamingStyle Style) {
   static llvm::Regex Matchers[] = {
if (NamingStyles[SK_TemplateParameter].isSet())
       return SK_TemplateParameter;
 
     return SK_Invalid;
   }
 
   return SK_Invalid;
 }
 
-void IdentifierNamingCheck::check(const MatchFinder::MatchResult &Result) {
-  if (const auto *DeclRef = Result.Nodes.getNodeAs<DeclRefExpr>("declref")) {
-    auto It = NamingCheckFailures.find(DeclRef->getDecl());
-    if (It == NamingCheckFailures.end())
-      return;
+static void addUsage(IdentifierNamingCheck::NamingCheckFailureMap &Failures,
+                     const NamedDecl *Decl, SourceRange Range,
+                     const SourceManager *SM) {
+  auto &Failure = Failures[Decl];
+  if (!Failure.RawUsageLocs.insert(Range.getBegin().getRawEncoding()).second)
 
-    NamingCheckFailure &Failure = It->second;
-    SourceRange Range = DeclRef->getNameInfo().getSourceRange();
-
-    Failure.Usages.push_back(Range);
-    Failure.ShouldFix = Failure.ShouldFix &&
-                        Result.SourceManager->isInMainFile(Range.getBegin()) &&
-                        Result.SourceManager->isInMainFile(Range.getEnd()) &&
-                        !Range.getBegin().isMacroID() &&
-                        !Range.getEnd().isMacroID();
+  Failure.ShouldFix = Failure.ShouldFix && SM->isInMainFile(Range.getBegin()) &&
+                      SM->isInMainFile(Range.getEnd()) &&
+}
 
-    return;
+void IdentifierNamingCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *DeclRef = Result.Nodes.getNodeAs<DeclRefExpr>("declref")) {
+    SourceRange Range = DeclRef->getNameInfo().getSourceRange();
+    return addUsage(NamingCheckFailures, DeclRef->getDecl(), Range,
+                    Result.SourceManager);
   }
 
   if (const auto *Decl = Result.Nodes.getNodeAs<NamedDecl>("decl")) {
     if (!Decl->getIdentifier() || Decl->getName().empty() || Decl->isImplicit())
       return;
 
     StyleKind SK = findStyleKind(Decl, NamingStyles);
     if (SK == SK_Invalid)
if (const auto *Decl = Result.Nodes.getNodeAs<NamedDecl>("decl")) {
     } else {
       NamingCheckFailure &Failure = NamingCheckFailures[Decl];
       SourceRange Range =
           DeclarationNameInfo(Decl->getDeclName(), Decl->getLocation())
               .getSourceRange();
 
       Failure.Fixup = std::move(Fixup);
       Failure.KindName = std::move(KindName);
-      Failure.ShouldFix =
-          Failure.ShouldFix &&
-          Result.SourceManager->isInMainFile(Range.getBegin()) &&
-          Result.SourceManager->isInMainFile(Range.getEnd()) &&
-          !Range.getBegin().isMacroID() && !Range.getEnd().isMacroID();
+      addUsage(NamingCheckFailures, Decl, Range, Result.SourceManager);
     }
   }
 }
 
 void IdentifierNamingCheck::onEndOfTranslationUnit() {
   for (const auto &Pair : NamingCheckFailures) {
     const NamedDecl &Decl = *Pair.first;
     const NamingCheckFailure &Failure = Pair.second;
 
-    SourceRange DeclRange =
-        DeclarationNameInfo(Decl.getDeclName(), Decl.getLocation())
-            .getSourceRange();
+    if (Failure.KindName.empty())
+      continue;
+
     auto Diag = diag(Decl.getLocStart(), "invalid case style for %0 '%1'")
                 << Failure.KindName << Decl.getName();
-
     if (Failure.ShouldFix) {
+      for (const auto &Loc : Failure.RawUsageLocs) {
+        // We assume that the identifier name is made of one token only. This is
+        // always the case as we ignore usages in macros that could build
+        // identifier names by combining multiple tokens.
         Diag << FixItHint::CreateReplacement(
-          CharSourceRange::getTokenRange(DeclRange), Failure.Fixup);
-
-      for (const auto &Range : Failure.Usages) {
-        Diag << FixItHint::CreateReplacement(
-            CharSourceRange::getTokenRange(Range), Failure.Fixup);
+            SourceRange(SourceLocation::getFromRawEncoding(Loc)),
+            Failure.Fixup);
       }
     }
   }
 }
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang

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

 // RUN:     {key: readability-identifier-naming.VirtualMethodPrefix, value: 'v_'}, \
 // RUN:     {key: readability-identifier-naming.IgnoreFailedSplit, value: 0} \
 // RUN:   ]}' -- -std=c++11 -fno-delayed-template-parsing
 
 // FIXME: There should be more test cases for checking that references to class
 // FIXME: name, declaration contexts, forward declarations, etc, are correctly
 // FIXME: checked and renamed
 
+// clang-format off
+
 namespace FOO_NS {
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: invalid case style for namespace 'FOO_NS' [readability-identifier-naming]
 // CHECK-FIXES: {{^}}namespace foo_ns {{{$}}
 inline namespace InlineNamespace {
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: invalid case style for inline namespace 'InlineNamespace'
 // CHECK-FIXES: {{^}}inline namespace inline_namespace {{{$}}
 
 #define BLA int FOO_bar