flx 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());
+ }
----------------
ymandel wrote:
> 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));
> }
> ```
Thanks for the snippet! I tried, but ran into the issue that llvm::Regex can
only be moved, but AST_MATCHER_P uses copy construction of its param somewhere.
I was able to work around it by using shared ptrs:
```
struct RegexAndScope {
llvm::Regex regex;
bool matchQualifiedName = false;
};
AST_MATCHER_P(NamedDecl, matchesAnyListedNameImpl,
std::vector<std::shared_ptr<RegexAndScope>>, Matchers) {
return llvm::any_of(
Matchers, [&Node](const std::shared_ptr<RegexAndScope>& Matcher) {
if (Matcher->matchQualifiedName) {
return Matcher->regex.match(Node.getQualifiedNameAsString());
}
return Matcher->regex.match(Node.getName());
});
}
ast_matchers::internal::Matcher<NamedDecl> matchesAnyListedName(
const std::vector<std::string>& NameList) {
std::vector<std::shared_ptr<RegexAndScope>> Matchers;
std::transform(NameList.begin(), NameList.end(),
std::back_inserter(Matchers),
[](const std::string& Name) {
auto RAS = std::make_shared<RegexAndScope>();
RAS->regex = llvm::Regex(Name);
RAS->matchQualifiedName =
Name.find("::") != std::string::npos;
return RAS;
});
return matchesAnyListedNameImpl(std::move(Matchers));
}
```
What do you think?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98738/new/
https://reviews.llvm.org/D98738
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
