[clang-tidy] readability-identifier-naming checks configs for include…

…d files When checking for the style of a decl that isn't in the main file, the check will now search for the configuration that the included files uses to gather the style for its decls. This can be useful to silence warnings in header files that follow a different naming convention without using header-filter to silence all warnings(even from other checks) in the header file. Reviewed By: aaron.ballman, gribozavr2 Differential Revision: https://reviews.llvm.org/D84814
llvm · Aug 1, 2020 · 4888c9c · 4888c9c
1 parent 75f134e
commit 4888c9c
Show file tree

Hide file tree

Showing 11 changed files with 201 additions and 68 deletions.
diff --git a/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp b/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -8,14 +8,16 @@
 
 #include "IdentifierNamingCheck.h"
 
+#include "../GlobList.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Preprocessor.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMapInfo.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Error.h"
-#include "llvm/Support/Format.h"
+#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
 
 #define DEBUG_TYPE "clang-tidy"
@@ -119,41 +121,47 @@ static StringRef const StyleNames[] = {
 #undef NAMING_KEYS
 // clang-format on
 
+static std::vector<llvm::Optional<IdentifierNamingCheck::NamingStyle>>
+getNamingStyles(const ClangTidyCheck::OptionsView &Options) {
+  std::vector<llvm::Optional<IdentifierNamingCheck::NamingStyle>> Styles;
+  Styles.reserve(StyleNames->size());
+  for (auto const &StyleName : StyleNames) {
+    auto CaseOptional = Options.getOptional<IdentifierNamingCheck::CaseType>(
+        (StyleName + "Case").str());
+    auto Prefix = Options.get((StyleName + "Prefix").str(), "");
+    auto Postfix = Options.get((StyleName + "Suffix").str(), "");
+
+    if (CaseOptional || !Prefix.empty() || !Postfix.empty())
+      Styles.emplace_back(IdentifierNamingCheck::NamingStyle{
+          std::move(CaseOptional), std::move(Prefix), std::move(Postfix)});
+    else
+      Styles.emplace_back(llvm::None);
+  }
+  return Styles;
+}
+
 IdentifierNamingCheck::IdentifierNamingCheck(StringRef Name,
                                              ClangTidyContext *Context)
-    : RenamerClangTidyCheck(Name, Context),
+    : RenamerClangTidyCheck(Name, Context), Context(Context), CheckName(Name),
+      GetConfigPerFile(Options.get("GetConfigPerFile", true)),
       IgnoreFailedSplit(Options.get("IgnoreFailedSplit", false)),
       IgnoreMainLikeFunctions(Options.get("IgnoreMainLikeFunctions", false)) {
 
-  for (auto const &Name : StyleNames) {
-    auto CaseOptional = [&]() -> llvm::Optional<CaseType> {
-      auto ValueOr = Options.get<CaseType>((Name + "Case").str());
-      if (ValueOr)
-        return *ValueOr;
-      llvm::logAllUnhandledErrors(
-          llvm::handleErrors(ValueOr.takeError(),
-                             [](const MissingOptionError &) -> llvm::Error {
-                               return llvm::Error::success();
-                             }),
-          llvm::errs(), "warning: ");
-      return llvm::None;
-    }();
-
-    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);
-    }
-  }
+  auto IterAndInserted = NamingStylesCache.try_emplace(
+      llvm::sys::path::parent_path(Context->getCurrentFile()),
+      getNamingStyles(Options));
+  assert(IterAndInserted.second && "Couldn't insert Style");
+  // Holding a reference to the data in the vector is safe as it should never
+  // move.
+  MainFileStyle = IterAndInserted.first->getValue();
 }
 
 IdentifierNamingCheck::~IdentifierNamingCheck() = default;
 
 void IdentifierNamingCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   RenamerClangTidyCheck::storeOptions(Opts);
+  ArrayRef<llvm::Optional<NamingStyle>> NamingStyles =
+      getStyleForFile(Context->getCurrentFile());
   for (size_t i = 0; i < SK_Count; ++i) {
     if (NamingStyles[i]) {
       if (NamingStyles[i]->Case) {
@@ -166,7 +174,7 @@ void IdentifierNamingCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
                     NamingStyles[i]->Suffix);
     }
   }
-
+  Options.store(Opts, "GetConfigPerFile", GetConfigPerFile);
   Options.store(Opts, "IgnoreFailedSplit", IgnoreFailedSplit);
   Options.store(Opts, "IgnoreMainLikeFunctions", IgnoreMainLikeFunctions);
 }
@@ -374,8 +382,7 @@ fixupWithStyle(StringRef Name,
 
 static StyleKind findStyleKind(
     const NamedDecl *D,
-    const std::vector<llvm::Optional<IdentifierNamingCheck::NamingStyle>>
-        &NamingStyles,
+    ArrayRef<llvm::Optional<IdentifierNamingCheck::NamingStyle>> NamingStyles,
     bool IgnoreMainLikeFunctions) {
   assert(D && D->getIdentifier() && !D->getName().empty() && !D->isImplicit() &&
          "Decl must be an explicit identifier with a name.");
@@ -652,63 +659,56 @@ static StyleKind findStyleKind(
   return SK_Invalid;
 }
 
-llvm::Optional<RenamerClangTidyCheck::FailureInfo>
-IdentifierNamingCheck::GetDeclFailureInfo(const NamedDecl *Decl,
-                                          const SourceManager &SM) const {
-  StyleKind SK = findStyleKind(Decl, NamingStyles, IgnoreMainLikeFunctions);
-  if (SK == SK_Invalid)
+static llvm::Optional<RenamerClangTidyCheck::FailureInfo> getFailureInfo(
+    StringRef Name, SourceLocation Location,
+    ArrayRef<llvm::Optional<IdentifierNamingCheck::NamingStyle>> NamingStyles,
+    StyleKind SK, const SourceManager &SM, bool IgnoreFailedSplit) {
+  if (SK == SK_Invalid || !NamingStyles[SK])
     return None;
 
-  if (!NamingStyles[SK])
-    return None;
-
-  const NamingStyle &Style = *NamingStyles[SK];
-  StringRef Name = Decl->getName();
+  const IdentifierNamingCheck::NamingStyle &Style = *NamingStyles[SK];
   if (matchesStyle(Name, Style))
     return None;
 
-  std::string KindName = fixupWithCase(StyleNames[SK], CT_LowerCase);
+  std::string KindName =
+      fixupWithCase(StyleNames[SK], IdentifierNamingCheck::CT_LowerCase);
   std::replace(KindName.begin(), KindName.end(), '_', ' ');
 
   std::string Fixup = fixupWithStyle(Name, Style);
   if (StringRef(Fixup).equals(Name)) {
     if (!IgnoreFailedSplit) {
-      LLVM_DEBUG(llvm::dbgs()
-                 << Decl->getBeginLoc().printToString(SM)
-                 << llvm::format(": unable to split words for %s '%s'\n",
-                                 KindName.c_str(), Name.str().c_str()));
+      LLVM_DEBUG(Location.print(llvm::dbgs(), SM);
+                 llvm::dbgs()
+                 << llvm::formatv(": unable to split words for {0} '{1}'\n",
+                                  KindName, Name));
     }
     return None;
   }
-  return FailureInfo{std::move(KindName), std::move(Fixup)};
+  return RenamerClangTidyCheck::FailureInfo{std::move(KindName),
+                                            std::move(Fixup)};
+}
+
+llvm::Optional<RenamerClangTidyCheck::FailureInfo>
+IdentifierNamingCheck::GetDeclFailureInfo(const NamedDecl *Decl,
+                                          const SourceManager &SM) const {
+  SourceLocation Loc = Decl->getLocation();
+  ArrayRef<llvm::Optional<NamingStyle>> NamingStyles =
+      getStyleForFile(SM.getFilename(Loc));
+
+  return getFailureInfo(
+      Decl->getName(), Loc, NamingStyles,
+      findStyleKind(Decl, NamingStyles, IgnoreMainLikeFunctions), SM,
+      IgnoreFailedSplit);
 }
 
 llvm::Optional<RenamerClangTidyCheck::FailureInfo>
 IdentifierNamingCheck::GetMacroFailureInfo(const Token &MacroNameTok,
                                            const SourceManager &SM) const {
-  if (!NamingStyles[SK_MacroDefinition])
-    return None;
-
-  StringRef Name = MacroNameTok.getIdentifierInfo()->getName();
-  const NamingStyle &Style = *NamingStyles[SK_MacroDefinition];
-  if (matchesStyle(Name, Style))
-    return None;
+  SourceLocation Loc = MacroNameTok.getLocation();
 
-  std::string KindName =
-      fixupWithCase(StyleNames[SK_MacroDefinition], CT_LowerCase);
-  std::replace(KindName.begin(), KindName.end(), '_', ' ');
-
-  std::string Fixup = fixupWithStyle(Name, Style);
-  if (StringRef(Fixup).equals(Name)) {
-    if (!IgnoreFailedSplit) {
-      LLVM_DEBUG(llvm::dbgs()
-                 << MacroNameTok.getLocation().printToString(SM)
-                 << llvm::format(": unable to split words for %s '%s'\n",
-                                 KindName.c_str(), Name.str().c_str()));
-    }
-    return None;
-  }
-  return FailureInfo{std::move(KindName), std::move(Fixup)};
+  return getFailureInfo(MacroNameTok.getIdentifierInfo()->getName(), Loc,
+                        getStyleForFile(SM.getFilename(Loc)),
+                        SK_MacroDefinition, SM, IgnoreFailedSplit);
 }
 
 RenamerClangTidyCheck::DiagInfo
@@ -720,6 +720,21 @@ IdentifierNamingCheck::GetDiagInfo(const NamingCheckId &ID,
                   }};
 }
 
+ArrayRef<llvm::Optional<IdentifierNamingCheck::NamingStyle>>
+IdentifierNamingCheck::getStyleForFile(StringRef FileName) const {
+  if (!GetConfigPerFile)
+    return MainFileStyle;
+  auto &Styles = NamingStylesCache[llvm::sys::path::parent_path(FileName)];
+  if (Styles.empty()) {
+    ClangTidyOptions Options = Context->getOptionsForFile(FileName);
+    if (Options.Checks && GlobList(*Options.Checks).contains(CheckName))
+      Styles = getNamingStyles({CheckName, Options.CheckOptions});
+    else
+      Styles.resize(SK_Count, None);
+  }
+  return Styles;
+}
+
 } // namespace readability
 } // namespace tidy
 } // namespace clang
diff --git a/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h b/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
@@ -10,6 +10,7 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_IDENTIFIERNAMINGCHECK_H
 
 #include "../utils/RenamerClangTidyCheck.h"
+#include "llvm/ADT/Optional.h"
 namespace clang {
 
 class MacroInfo;
@@ -69,7 +70,17 @@ class IdentifierNamingCheck final : public RenamerClangTidyCheck {
   DiagInfo GetDiagInfo(const NamingCheckId &ID,
                        const NamingCheckFailure &Failure) const override;
 
-  std::vector<llvm::Optional<NamingStyle>> NamingStyles;
+  ArrayRef<llvm::Optional<NamingStyle>>
+  getStyleForFile(StringRef FileName) const;
+
+  /// Stores the style options as a vector, indexed by the specified \ref
+  /// StyleKind, for a given directory.
+  mutable llvm::StringMap<std::vector<llvm::Optional<NamingStyle>>>
+      NamingStylesCache;
+  ArrayRef<llvm::Optional<NamingStyle>> MainFileStyle;
+  ClangTidyContext *const Context;
+  const std::string CheckName;
+  const bool GetConfigPerFile;
   const bool IgnoreFailedSplit;
   const bool IgnoreMainLikeFunctions;
 };

diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -67,7 +67,14 @@ The improvements are...
 Improvements to clang-tidy
 --------------------------
 
-The improvements are...
+Changes in existing checks
+^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+- Improved :doc:`readability-identifier-naming
+  <clang-tidy/checks/readability-identifier-naming>` check.
+
+  Added an option `GetConfigPerFile` to support including files which use
+  different naming styles.
 
 Improvements to include-fixer
 -----------------------------

diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst b/clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst
@@ -51,6 +51,7 @@ The following options are describe below:
  - :option:`EnumCase`, :option:`EnumPrefix`, :option:`EnumSuffix`
  - :option:`EnumConstantCase`, :option:`EnumConstantPrefix`, :option:`EnumConstantSuffix`
  - :option:`FunctionCase`, :option:`FunctionPrefix`, :option:`FunctionSuffix`
+ - :option:`GetConfigPerFile`
  - :option:`GlobalConstantCase`, :option:`GlobalConstantPrefix`, :option:`GlobalConstantSuffix`
  - :option:`GlobalConstantPointerCase`, :option:`GlobalConstantPointerPrefix`, :option:`GlobalConstantPointerSuffix`
  - :option:`GlobalFunctionCase`, :option:`GlobalFunctionPrefix`, :option:`GlobalFunctionSuffix`
@@ -713,6 +714,13 @@ After:
 
     char pre_my_function_string_post();
 
+.. option:: GetConfigPerFile
+
+    When `true` the check will look for the configuration for where an
+    identifier is declared. Useful for when included header files use a 
+    different style. 
+    Default value is `true`.
+
 .. option:: GlobalConstantCase
 
     When defined, the check will ensure global constant names conform to the

diff --git a/...lang-tidy/checkers/Inputs/readability-identifier-naming/global-style-disabled/.clang-tidy b/...lang-tidy/checkers/Inputs/readability-identifier-naming/global-style-disabled/.clang-tidy
@@ -0,0 +1,5 @@
+Checks: -readability-identifier-naming
+CheckOptions:
+  - key:             readability-identifier-naming.GlobalFunctionCase
+    value:           lower_case
+
diff --git a/...t/clang-tidy/checkers/Inputs/readability-identifier-naming/global-style-disabled/header.h b/...t/clang-tidy/checkers/Inputs/readability-identifier-naming/global-style-disabled/header.h
@@ -0,0 +1,3 @@
+void disabled_style_1();
+void disabledStyle2();
+void DISABLED_STYLE_3();
diff --git a/...a/test/clang-tidy/checkers/Inputs/readability-identifier-naming/global-style1/.clang-tidy b/...a/test/clang-tidy/checkers/Inputs/readability-identifier-naming/global-style1/.clang-tidy
@@ -0,0 +1,5 @@
+Checks: readability-identifier-naming
+CheckOptions:
+  - key:             readability-identifier-naming.GlobalFunctionCase
+    value:           lower_case
+
diff --git a/...xtra/test/clang-tidy/checkers/Inputs/readability-identifier-naming/global-style1/header.h b/...xtra/test/clang-tidy/checkers/Inputs/readability-identifier-naming/global-style1/header.h
@@ -0,0 +1,5 @@
+
+
+void style_first_good();
+
+void styleFirstBad();
diff --git a/...a/test/clang-tidy/checkers/Inputs/readability-identifier-naming/global-style2/.clang-tidy b/...a/test/clang-tidy/checkers/Inputs/readability-identifier-naming/global-style2/.clang-tidy
@@ -0,0 +1,5 @@
+Checks: readability-identifier-naming
+CheckOptions:
+  - key:             readability-identifier-naming.GlobalFunctionCase
+    value:           UPPER_CASE
+
diff --git a/...xtra/test/clang-tidy/checkers/Inputs/readability-identifier-naming/global-style2/header.h b/...xtra/test/clang-tidy/checkers/Inputs/readability-identifier-naming/global-style2/header.h
@@ -0,0 +1,5 @@
+
+
+void STYLE_SECOND_GOOD();
+
+void styleSecondBad();
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-multiple-styles.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-multiple-styles.cpp
@@ -0,0 +1,64 @@
+// Setup header directory
+
+// RUN: rm -rf %theaders
+// RUN: mkdir %theaders
+// RUN: cp -R %S/Inputs/readability-identifier-naming/. %theaders
+
+// C++11 isn't explicitly required, but failing to specify a standard means the
+// check will run multiple times for different standards. This will cause the
+// second test to fail as the header file will be changed during the first run.
+// InheritParentConfig is needed to look for the clang-tidy configuration files.
+
+// RUN: %check_clang_tidy -check-suffixes=ENABLED,SHARED -std=c++11 %s \
+// RUN: readability-identifier-naming %t -- \
+// RUN:  -config='{ InheritParentConfig: true, CheckOptions: [ \
+// RUN:   {key: readability-identifier-naming.FunctionCase, value: camelBack}, \
+// RUN:   {key: readability-identifier-naming.GetConfigPerFile, value: true} \
+// RUN:  ]}' -header-filter='.*' -- -I%theaders
+
+// On DISABLED run, everything should be made 'camelBack'.
+
+// RUN: cp -R %S/Inputs/readability-identifier-naming/. %theaders
+// RUN: %check_clang_tidy -check-suffixes=DISABLED,SHARED -std=c++11 %s \
+// RUN: readability-identifier-naming %t -- \
+// RUN:  -config='{ InheritParentConfig: true, CheckOptions: [ \
+// RUN:   {key: readability-identifier-naming.FunctionCase, value: camelBack}, \
+// RUN:   {key: readability-identifier-naming.GetConfigPerFile, value: false} \
+// RUN:  ]}' -header-filter='.*' -- -I%theaders
+
+#include "global-style-disabled/header.h"
+#include "global-style1/header.h"
+#include "global-style2/header.h"
+// CHECK-MESSAGES-ENABLED-DAG: global-style1/header.h:5:6: warning: invalid case style for global function 'styleFirstBad'
+// CHECK-MESSAGES-ENABLED-DAG: global-style2/header.h:5:6: warning: invalid case style for global function 'styleSecondBad'
+// CHECK-MESSAGES-DISABLED-DAG: global-style1/header.h:3:6: warning: invalid case style for function 'style_first_good'
+// CHECK-MESSAGES-DISABLED-DAG: global-style2/header.h:3:6: warning: invalid case style for function 'STYLE_SECOND_GOOD'
+// CHECK-MESSAGES-DISABLED-DAG: global-style-disabled/header.h:1:6: warning: invalid case style for function 'disabled_style_1'
+// CHECK-MESSAGES-DISABLED-DAG: global-style-disabled/header.h:3:6: warning: invalid case style for function 'DISABLED_STYLE_3'
+
+void goodStyle() {
+  style_first_good();
+  STYLE_SECOND_GOOD();
+  //      CHECK-FIXES-DISABLED: styleFirstGood();
+  // CHECK-FIXES-DISABLED-NEXT: styleSecondGood();
+}
+// CHECK-MESSAGES-SHARED-DAG: :[[@LINE+1]]:6: warning: invalid case style for function 'bad_style'
+void bad_style() {
+  styleFirstBad();
+  styleSecondBad();
+}
+//        CHECK-FIXES-SHARED: void badStyle() {
+// CHECK-FIXES-DISABLED-NEXT:   styleFirstBad();
+//  CHECK-FIXES-ENABLED-NEXT:   style_first_bad();
+// CHECK-FIXES-DISABLED-NEXT:   styleSecondBad();
+//  CHECK-FIXES-ENABLED-NEXT:   STYLE_SECOND_BAD();
+//   CHECK-FIXES-SHARED-NEXT: }
+
+void expectNoStyle() {
+  disabled_style_1();
+  disabledStyle2();
+  DISABLED_STYLE_3();
+  //      CHECK-FIXES-DISABLED: disabledStyle1();
+  // CHECK-FIXES-DISABLED-NEXT: disabledStyle2();
+  // CHECK-FIXES-DISABLED-NEXT: disabledStyle3();
+}