[clang-tidy] Use compiled regex for AllowedRegexp in macro usage check

[smhc]

Summary

Current check compiles the regex on every attempt at matching. The check also populates and enables a regex value by default so the default behaviour results in regex re-compilation for every macro - if the check is enabled. If people used this check there's a reasonable chance they would have relatively complex regexes in use.

This is a quick and simple fix to store and use the compiled regex.

[Eugene.Zelenko]

Please add test case for new command-line option.

[njames93]

While I'm a fan of this change, I think this is the wrong way to do it. Leave the check the same, but build the regex in the pp callback constructor. That should only get called once for the lifetime of the check.

[smhc]

Thanks - I had it like that originally but was not sure how often MacroUsageCallbacks would be instantiated or whether there was a many-to-one relationship. I've now modified it to create and maintain the regex object as a member of MacroUsageCallbacks.

A test would require a unit test or a performance test - I don't think this is practical without an existing framework.

[njames93]

Because this is basically NFC and we already have tests for the AllowedRegexp option, there is no need to introduce new tests here.
So long as those current tests pass with this change, this should be perfectly fine.

[smhc]

Thank you - I don't have commit access so am unable to merge it myself. The clang-tidy tests are passing fine.

[njames93]

In D91908#2410240, @smhc wrote:

Thank you - I don't have commit access so am unable to merge it myself. The clang-tidy tests are passing fine.

Just checking, Has this landed correctly, showing you are the author? I'm assuming https://github.com/smhc is correct.

[smhc]

Yes all looks good, thanks!

clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp

if (std::isupper(c) || std::isdigit(c) || c == '_')
       return true;
     return false;
   });
 }
 
 class MacroUsageCallbacks : public PPCallbacks {
 public:
   MacroUsageCallbacks(MacroUsageCheck *Check, const SourceManager &SM,
-                      StringRef RegExp, bool CapsOnly, bool IgnoreCommandLine)
-      : Check(Check), SM(SM), RegExp(RegExp), CheckCapsOnly(CapsOnly),
+                      StringRef RegExpStr, bool CapsOnly, bool IgnoreCommandLine)
+      : Check(Check), SM(SM), RegExp(RegExpStr), CheckCapsOnly(CapsOnly),
         IgnoreCommandLineMacros(IgnoreCommandLine) {}
   void MacroDefined(const Token &MacroNameTok,
                     const MacroDirective *MD) override {
     if (SM.isWrittenInBuiltinFile(MD->getLocation()) ||
         MD->getMacroInfo()->isUsedForHeaderGuard() ||
         MD->getMacroInfo()->getNumTokens() == 0)
       return;
 
     if (IgnoreCommandLineMacros &&
         SM.isWrittenInCommandLineFile(MD->getLocation()))
       return;
 
     StringRef MacroName = MacroNameTok.getIdentifierInfo()->getName();
-    if (!CheckCapsOnly && !llvm::Regex(RegExp).match(MacroName))
+    if (!CheckCapsOnly && !RegExp.match(MacroName))
       Check->warnMacro(MD, MacroName);
 
     if (CheckCapsOnly && !isCapsOnly(MacroName))
       Check->warnNaming(MD, MacroName);
   }
 
 private:
   MacroUsageCheck *Check;
   const SourceManager &SM;
-  StringRef RegExp;
+  const llvm::Regex RegExp;
   bool CheckCapsOnly;
   bool IgnoreCommandLineMacros;
 };
 } // namespace
 
 void MacroUsageCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "AllowedRegexp", AllowedRegexp);
   Options.store(Opts, "CheckCapsOnly", CheckCapsOnly);