[clang-tidy] Reserved-identifier: Improved AllowedIdentifiers option to support regular expressions

[felix642]

Summary

Fixes: https://github.com/llvm/llvm-project/issues/59119

[logan-5]

Thanks for the patch. This seems like it'll make the check much more useful.

clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp

48

Nit: we should AllowedIdentifiersRegex.reserve(AllowedIdentifiers.size()) before the loop.

clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.h

34

I understand that we need to keep the AllowedIdentifiers member around for use in storeOptions. To avoid confusion, it might make sense to change its name to be the "less default" name (something like AllowedIdentifiersRaw), and give the vector of regexes the more intuitive name AllowedIdentifiers. That would further avoid confusion since getFailureInfoImpl in the .cpp currently refers to the vector of regexes using the name AllowedIdentifiers.

Also, this should probably be const for consistency. The loop in the constructor body should arguably be moved into a helper function anyway, so this member can be initialized in the constructor initializer list.

[PiotrZSL]

Except pointed out issues, looks fine.

clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp

49–50

avoid double compilation of regexp.
Simply:

AllowedIdentifiersRegex.emplace_back(Identifier.str());
if (!AllowedIdentifiersRegex.back().isValid()) {
       AllowedIdentifiersRegex.pop_back();
    configurationDiag("Invalid allowed identifier regex '%0'") << Identifier;
}

clang-tools-extra/docs/ReleaseNotes.rst

414–416

Sort this entry by full check name.

[felix642]

Renamed allowedIdentifiers to allowedIdentifiersRaw
Moved regex parsing to a new method
Moved entry in releaseNotes.rst

[PiotrZSL]

Overall looks good..

clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp

50

you may remove that last one that is not valid from an AllowedIdentifiers, simply use pop_back()

clang-tools-extra/docs/ReleaseNotes.rst

246

Consider following current approach: "Improved the bugprone-reserved-identifier check by enhancing the AllowedIdentifiers option to support regular expressions."

247

consider inserting here "check"

[felix642]

Resolved comments.

[PiotrZSL]

LGTM

clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.h

 /// underscore, while the C++ standard strengthens this to reserve names with a
 /// double underscore occurring anywhere.
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/reserved-identifier.html
 class ReservedIdentifierCheck final : public RenamerClangTidyCheck {
   const bool Invert;
   const std::vector<StringRef> AllowedIdentifiers;
+  llvm::SmallVector<llvm::Regex> AllowedIdentifiersRegex;
 
 public:
   ReservedIdentifierCheck(StringRef Name, ClangTidyContext *Context);
 
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
 
 private:
   std::optional<FailureInfo>

clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp

static int getMessageSelectIndex(StringRef Tag) {
   return 0;
 }
 
 ReservedIdentifierCheck::ReservedIdentifierCheck(StringRef Name,
                                                  ClangTidyContext *Context)
     : RenamerClangTidyCheck(Name, Context),
       Invert(Options.get("Invert", false)),
       AllowedIdentifiers(utils::options::parseStringList(
-          Options.get("AllowedIdentifiers", ""))) {}
+          Options.get("AllowedIdentifiers", ""))) {
+  for (const auto &Identifier : AllowedIdentifiers) {
+    if (!llvm::Regex(Identifier).isValid())
+      configurationDiag("Invalid allowed identifier regex '%0'") << Identifier;
+    AllowedIdentifiersRegex.emplace_back(Identifier.str());
+  }
+}
 
 void ReservedIdentifierCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   RenamerClangTidyCheck::storeOptions(Opts);
   Options.store(Opts, "Invert", Invert);
   Options.store(Opts, "AllowedIdentifiers",
                 utils::options::serializeStringList(AllowedIdentifiers));
 }
 
static std::string getNonReservedFixup(std::string Name) {
   else
     Name.insert(Name.begin(), 2, '_');
   return Name;
 }
 
 static std::optional<RenamerClangTidyCheck::FailureInfo>
 getFailureInfoImpl(StringRef Name, bool IsInGlobalNamespace,
                    const LangOptions &LangOpts, bool Invert,
-                   ArrayRef<StringRef> AllowedIdentifiers) {
+                   ArrayRef<llvm::Regex> AllowedIdentifiers) {
   assert(!Name.empty());
-  if (llvm::is_contained(AllowedIdentifiers, Name))
-    return std::nullopt;
 
+  if (llvm::any_of(AllowedIdentifiers, [&](const llvm::Regex &Regex) {
+        return Regex.match(Name);
+      })) {
+    return std::nullopt;
+  }
   // TODO: Check for names identical to language keywords, and other names
   // specifically reserved by language standards, e.g. C++ 'zombie names' and C
   // future library directions
 
   using FailureInfo = RenamerClangTidyCheck::FailureInfo;
   if (!Invert) {
     std::optional<FailureInfo> Info;
     auto AppendFailure = [&](StringRef Kind, std::string &&Fixup) {

 std::optional<RenamerClangTidyCheck::FailureInfo>
 ReservedIdentifierCheck::getDeclFailureInfo(const NamedDecl *Decl,
                                             const SourceManager &) const {
   assert(Decl && Decl->getIdentifier() && !Decl->getName().empty() &&
          !Decl->isImplicit() &&
          "Decl must be an explicit identifier with a name.");
   return getFailureInfoImpl(Decl->getName(),
                             isa<TranslationUnitDecl>(Decl->getDeclContext()),
-                            getLangOpts(), Invert, AllowedIdentifiers);
+                            getLangOpts(), Invert, AllowedIdentifiersRegex);
 }
 
 std::optional<RenamerClangTidyCheck::FailureInfo>
 ReservedIdentifierCheck::getMacroFailureInfo(const Token &MacroNameTok,
                                              const SourceManager &) const {
   return getFailureInfoImpl(MacroNameTok.getIdentifierInfo()->getName(), true,
-                            getLangOpts(), Invert, AllowedIdentifiers);
+                            getLangOpts(), Invert, AllowedIdentifiersRegex);
 }
 
 RenamerClangTidyCheck::DiagInfo
 ReservedIdentifierCheck::getDiagInfo(const NamingCheckId &ID,
                                      const NamingCheckFailure &Failure) const {
   return DiagInfo{Message, [&](DiagnosticBuilder &Diag) {
                     Diag << ID.second
                          << getMessageSelectIndex(Failure.Info.KindName);
                   }};
 }
 
 } // namespace clang::tidy::bugprone

clang-tools-extra/docs/ReleaseNotes.rst

 - Improved :doc:`bugprone-fold-init-type
   <clang-tidy/checks/bugprone/fold-init-type>` to handle iterators that do not
   define `value_type` type aliases.
 
 - Improved :doc:`bugprone-incorrect-roundings
   <clang-tidy/checks/bugprone/incorrect-roundings>` check by adding support for
   other floating point representations in float constant like ``0.5L``.
 
 - Deprecated check-local options `HeaderFileExtensions` and `ImplementationFileExtensions`
   in :doc:`bugprone-suspicious-include
   <clang-tidy/checks/bugprone/suspicious-include>` check.
   Global options of the same name should be used instead.
 
 - Improved :doc:`bugprone-too-small-loop-variable
   <clang-tidy/checks/bugprone/too-small-loop-variable>` check. Basic support
   for bit-field and integer members as a loop variable or upper limit were added.
 
 - Improved :doc:`bugprone-unchecked-optional-access
- Improved :doc:`readability-redundant-string-cstr
   arguments to ``std::print``, ``std::format`` or other functions listed in
   the ``StringParameterFunction`` check option.
 
 - Improved :doc:`readability-static-accessed-through-instance
   <clang-tidy/checks/readability/static-accessed-through-instance>` check to
   support unscoped enumerations through instances and fixed usage of anonymous
   structs or classes.
 
+- Improved option `AllowedIdentifiers` from :doc:`bugprone-reserved-identifier
+  <clang-tidy/checks/bugprone/reserved-identifier>` to support regular
+  expressions.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 

clang-tools-extra/docs/clang-tidy/checks/bugprone/reserved-identifier.rst

 
 .. option:: Invert
 
    If `true`, inverts the check, i.e. flags names that are not reserved.
    Default is `false`.
 
 .. option:: AllowedIdentifiers
 
-   Semicolon-separated list of names that the check ignores. Default is an
+   Semicolon-separated list of regular expressions that the check ignores. Default is an
    empty list.

clang-tools-extra/test/clang-tidy/checkers/bugprone/reserved-identifier-invert.cpp

 // RUN: %check_clang_tidy %s bugprone-reserved-identifier %t -- \
 // RUN:   -config='{CheckOptions: [ \
 // RUN:     {key: bugprone-reserved-identifier.Invert, value: true}, \
-// RUN:     {key: bugprone-reserved-identifier.AllowedIdentifiers, value: std;reference_wrapper;ref;cref;type;get}, \
+// RUN:     {key: bugprone-reserved-identifier.AllowedIdentifiers, value: "std;reference_wrapper;^c?ref;type;get"}, \
 // RUN:   ]}' -- \
 // RUN:   -I%S/Inputs/reserved-identifier \
 // RUN:   -isystem %S/Inputs/reserved-identifier/system
 
 namespace std {
 
 void __f() {}