[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

63

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

64–65

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

418–420

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

 /// The C standard additionally reserves names beginning with a double
 /// 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;
+  const std::vector<StringRef> AllowedIdentifiersRaw;
+  const llvm::SmallVector<llvm::Regex> AllowedIdentifiers;
 
 public:
   ReservedIdentifierCheck(StringRef Name, ClangTidyContext *Context);
 
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
 
 private:
   std::optional<FailureInfo>
   getDeclFailureInfo(const NamedDecl *Decl,
                      const SourceManager &SM) const override;
   std::optional<FailureInfo>
   getMacroFailureInfo(const Token &MacroNameTok,
                       const SourceManager &SM) const override;
   DiagInfo getDiagInfo(const NamingCheckId &ID,
                        const NamingCheckFailure &Failure) const override;
+  llvm::SmallVector<llvm::Regex> parseAllowedIdentifiers() const;
 };
 
 } // namespace clang::tidy::bugprone
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_RESERVEDIDENTIFIERCHECK_H

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

 static int getMessageSelectIndex(StringRef Tag) {
   if (Tag == NonReservedTag)
     return 1;
   if (Tag == GlobalUnderscoreTag)
     return 2;
   return 0;
 }
 
+llvm::SmallVector<llvm::Regex>
+ReservedIdentifierCheck::parseAllowedIdentifiers() const {
+  llvm::SmallVector<llvm::Regex> AllowedIdentifiers;
+  AllowedIdentifiers.reserve(AllowedIdentifiersRaw.size());
+
+  for (const auto &Identifier : AllowedIdentifiersRaw) {
+    AllowedIdentifiers.emplace_back(Identifier.str());
+    if (!AllowedIdentifiers.back().isValid()) {
+      configurationDiag("Invalid allowed identifier regex '%0'") << Identifier;
+      AllowedIdentifiers.pop_back();
+    }
+  }
+
+  return AllowedIdentifiers;
+}
+
 ReservedIdentifierCheck::ReservedIdentifierCheck(StringRef Name,
                                                  ClangTidyContext *Context)
     : RenamerClangTidyCheck(Name, Context),
       Invert(Options.get("Invert", false)),
-      AllowedIdentifiers(utils::options::parseStringList(
-          Options.get("AllowedIdentifiers", ""))) {}
+      AllowedIdentifiersRaw(utils::options::parseStringList(
+          Options.get("AllowedIdentifiers", ""))),
+      AllowedIdentifiers(parseAllowedIdentifiers()) {}
 
 void ReservedIdentifierCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   RenamerClangTidyCheck::storeOptions(Opts);
   Options.store(Opts, "Invert", Invert);
   Options.store(Opts, "AllowedIdentifiers",
-                utils::options::serializeStringList(AllowedIdentifiers));
+                utils::options::serializeStringList(AllowedIdentifiersRaw));
 }
 
 static std::string collapseConsecutive(StringRef Str, char C) {
   std::string Result;
   std::unique_copy(Str.begin(), Str.end(), std::back_inserter(Result),
                    [C](char A, char B) { return A == C && B == C; });
   return Result;
 }
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) {

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``.
 
+- Improved the :doc:`bugprone-reserved-identifier 
+  <clang-tidy/checks/bugprone/reserved-identifier>` check by enhancing the 
+  `AllowedIdentifiers` option to support regular expressions.
+
 - 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:`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.
 
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to clang-include-fixer
 -----------------------------------
 

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() {}