[clang-tidy] Code factorization and cleanup in IdentifierNamingCheck

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.

Patch by Beren Minor!

Differential revision: http://reviews.llvm.org/D13079

llvm-svn: 248700

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

@@ -21,6 +21,7 @@ namespace clang {
 namespace tidy {
 namespace readability {
 
+// clang-format off
 #define NAMING_KEYS(m) \
     m(Namespace) \
     m(InlineNamespace) \
@@ -80,6 +81,7 @@ static StringRef const StyleNames[] = {
 };
 
 #undef NAMING_KEYS
+// clang-format on
 
 IdentifierNamingCheck::IdentifierNamingCheck(StringRef Name,
                                              ClangTidyContext *Context)
@@ -134,10 +136,10 @@ void IdentifierNamingCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
 }
 
 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);
@@ -499,23 +501,24 @@ static StyleKind findStyleKind(
   return SK_Invalid;
 }
 
+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)
+    return;
+
+  Failure.ShouldFix = Failure.ShouldFix && SM->isInMainFile(Range.getBegin()) &&
+                      SM->isInMainFile(Range.getEnd()) &&
+                      !Range.getBegin().isMacroID() &&
+                      !Range.getEnd().isMacroID();
+}
+
 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;
-
-    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();
-
-    return;
+    return addUsage(NamingCheckFailures, DeclRef->getDecl(), Range,
+                    Result.SourceManager);
   }
 
   if (const auto *Decl = Result.Nodes.getNodeAs<NamedDecl>("decl")) {
@@ -550,11 +553,7 @@ void IdentifierNamingCheck::check(const MatchFinder::MatchResult &Result) {
 
       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);
     }
   }
 }
@@ -564,19 +563,19 @@ void IdentifierNamingCheck::onEndOfTranslationUnit() {
     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) {
-      Diag << FixItHint::CreateReplacement(
-          CharSourceRange::getTokenRange(DeclRange), Failure.Fixup);
-
-      for (const auto &Range : Failure.Usages) {
+      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(Range), Failure.Fixup);
+            SourceRange(SourceLocation::getFromRawEncoding(Loc)),
+            Failure.Fixup);
       }
     }
   }

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

@@ -62,20 +62,32 @@ public:
     }
   };
 
-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

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

@@ -68,6 +68,8 @@
 // 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 {{{$}}