[PATCH] D98738: [clang-tidy] performance-* checks: Match AllowedTypes against qualified type names when they contain "::".
Yitzhak Mandelbaum via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Mar 17 07:17:44 PDT 2021
ymandel added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/utils/Matchers.h:56
+ if (Name.find("::") != std::string::npos) {
+ return llvm::Regex(Name).match(Node.getQualifiedNameAsString());
+ }
----------------
flx wrote:
> ymandel wrote:
> > nit: while this change is not much worse than the existing code, the matcher really should perform this test for "::", and create the Regex, for each string just once. Regex construction is relatively expensive, and matchers of this sort (very generic) are often called quite frequently.
> >
> > for that matter, this matcher is now really close to `matchesName` and would probably be best as a first-class matcher, rather than limited to clang-tidy/utils. But, I'll admit that such is beyond the scope of this patch.
> It wouldn't be hard to change it something like this:
>
>
> ```
> struct MatcherPair {
> llvm::Regex regex;
> bool matchQualifiedName = false;
> };
> std::vector<MatcherPair> Matchers;
> std::transform(
> NameList.begin(), NameList.end(), std::back_inserter(Matchers),
> [](const std::string& Name) {
> return MatcherPair{
> .regex = llvm::Regex(Name),
> .matchQualifiedName = Name.find("::") != std::string::npos,
> };
> });
> return llvm::any_of(Matchers, [&Node](const MatcherPair& Matcher) {
> if (Matcher.matchQualifiedName) {
> return Matcher.regex.match(Node.getQualifiedNameAsString());
> }
> return Matcher.regex.match(Node.getName());
> });
> ```
>
> Is this what you had in mind?
Thanks, that's almost it. The key point is that the code before the `return` be executed once, during matcher construction, and not each time `match` is called. You could define your own class (instead of using the macro) or just your own factory function:
```
struct MatcherPair {
llvm::Regex regex;
bool matchQualifiedName = false;
};
AST_MATCHER_P(NamedDecl, matchesAnyListedNameImpl, std::vector<MatcherPair>, Matchers) {
return llvm::any_of(Matchers, [&Node](const MatcherPair& Matcher) {
if (Matcher.matchQualifiedName) {
return Matcher.regex.match(Node.getQualifiedNameAsString());
}
return Matcher.regex.match(Node.getName());
});
}
Matcher<NamedDecl> matchesAnyListedName(std::vector<std::string> NameList) {
std::vector<MatcherPair> Matchers;
std::transform(
NameList.begin(), NameList.end(), std::back_inserter(Matchers),
[](const std::string& Name) {
return MatcherPair{
.regex = llvm::Regex(Name),
.matchQualifiedName = Name.find("::") != std::string::npos,
};
});
return matchesAnyListNameImpl(std::move(Matchers));
}
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98738/new/
https://reviews.llvm.org/D98738
More information about the cfe-commits
mailing list