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

[Shane via Phabricator via cfe-commits]

smhc added a comment.

Yes all looks good, thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91908/new/

https://reviews.llvm.org/D91908

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91908/new/

https://reviews.llvm.org/D91908

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan James via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG269ef315d1be: [clang-tidy] Use compiled regex for 
AllowedRegexp in macro usage check (authored by smhc, committed by njames93).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91908/new/

https://reviews.llvm.org/D91908

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


Index: clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
@@ -32,8 +32,8 @@
 class MacroUsageCallbacks : public PPCallbacks {
 public:
   MacroUsageCallbacks(MacroUsageCheck *Check, const SourceManager ,
-  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 ,
 const MacroDirective *MD) override {
@@ -47,7 +47,7 @@
   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))
@@ -57,7 +57,7 @@
 private:
   MacroUsageCheck *Check;
   const SourceManager 
-  StringRef RegExp;
+  const llvm::Regex RegExp;
   bool CheckCapsOnly;
   bool IgnoreCommandLineMacros;
 };


Index: clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
@@ -32,8 +32,8 @@
 class MacroUsageCallbacks : public PPCallbacks {
 public:
   MacroUsageCallbacks(MacroUsageCheck *Check, const SourceManager ,
-  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 ,
 const MacroDirective *MD) override {
@@ -47,7 +47,7 @@
   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))
@@ -57,7 +57,7 @@
 private:
   MacroUsageCheck *Check;
   const SourceManager 
-  StringRef RegExp;
+  const llvm::Regex RegExp;
   bool CheckCapsOnly;
   bool IgnoreCommandLineMacros;
 };
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Shane via Phabricator via cfe-commits]

smhc added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91908/new/

https://reviews.llvm.org/D91908

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan James via Phabricator via cfe-commits]

njames93 accepted this revision.
njames93 added a comment.
This revision is now accepted and ready to land.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91908/new/

https://reviews.llvm.org/D91908

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Shane via Phabricator via cfe-commits]

smhc updated this revision to Diff 306870.
smhc added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91908/new/

https://reviews.llvm.org/D91908

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


Index: clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
@@ -32,8 +32,8 @@
 class MacroUsageCallbacks : public PPCallbacks {
 public:
   MacroUsageCallbacks(MacroUsageCheck *Check, const SourceManager ,
-  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 ,
 const MacroDirective *MD) override {
@@ -47,7 +47,7 @@
   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))
@@ -57,7 +57,7 @@
 private:
   MacroUsageCheck *Check;
   const SourceManager 
-  StringRef RegExp;
+  const llvm::Regex RegExp;
   bool CheckCapsOnly;
   bool IgnoreCommandLineMacros;
 };


Index: clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
@@ -32,8 +32,8 @@
 class MacroUsageCallbacks : public PPCallbacks {
 public:
   MacroUsageCallbacks(MacroUsageCheck *Check, const SourceManager ,
-  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 ,
 const MacroDirective *MD) override {
@@ -47,7 +47,7 @@
   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))
@@ -57,7 +57,7 @@
 private:
   MacroUsageCheck *Check;
   const SourceManager 
-  StringRef RegExp;
+  const llvm::Regex RegExp;
   bool CheckCapsOnly;
   bool IgnoreCommandLineMacros;
 };
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91908/new/

https://reviews.llvm.org/D91908

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please add test case for new command-line option.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91908/new/

https://reviews.llvm.org/D91908

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits