Page MenuHomePhabricator

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

Authored by smhc on Sat, Nov 21, 1:16 AM.

Details

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.

Diff Detail

Event Timeline

smhc created this revision.Sat, Nov 21, 1:16 AM
smhc requested review of this revision.Sat, Nov 21, 1:16 AM

Please add test case for new command-line option.

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 updated this revision to Diff 306870.EditedSat, Nov 21, 12:26 PM

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 accepted this revision.Sun, Nov 22, 5:18 AM

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.

This revision is now accepted and ready to land.Sun, Nov 22, 5:18 AM
smhc added a comment.Sun, Nov 22, 12:42 PM

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

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 added a comment.Mon, Nov 23, 3:16 PM

Yes all looks good, thanks!