hokein added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/utils/Matchers.h:52
AST_MATCHER_P(NamedDecl, matchesAnyListedName, std::vector<std::string>,
NameList) {
----------------
worth some comments on this.
================
Comment at: clang-tools-extra/clang-tidy/utils/Matchers.h:55
return llvm::any_of(NameList, [&Node](const std::string &Name) {
- return llvm::Regex(Name).match(Node.getName());
- });
+ if (Name.find("::") != std::string::npos) {
+ return llvm::Regex(Name).match(Node.getQualifiedNameAsString());
----------------
If we pass the name as a `llvm::StringRef`, we can use `if
(Name.contains("::"))` which seems a bit clearer to me.
================
Comment at: clang-tools-extra/clang-tidy/utils/Matchers.h:55
return llvm::any_of(NameList, [&Node](const std::string &Name) {
- return llvm::Regex(Name).match(Node.getName());
- });
+ if (Name.find("::") != std::string::npos) {
+ return llvm::Regex(Name).match(Node.getQualifiedNameAsString());
----------------
hokein wrote:
> If we pass the name as a `llvm::StringRef`, we can use `if
> (Name.contains("::"))` which seems a bit clearer to me.
there is a tricky case where the `::` is a prefix of the Name, e.g. a global
symbol x, the Name is `::x`, and `getQualifiedNameAsString` of symbol x is `x`
(without the `::` prefix), then the matcher will not find a match, but we
expect this is a match right?
================
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:
> > > 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?
> That works for me, but let's see what other reviewers think. Personally, I'd
> just define the class directly at this point:
> ```
> class MatchesAnyListedNameMatcher
> : public ast_matchers::internal::MatcherInterface<NamedDecl> {
> public:
> explicit MatchesAnyListedNameMatcher(llvm::ArrayRef<std::string> NameList) {
> 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,
> };
> });
> }
> bool matches(
> const NamedDecl& Node, ast_matchers::internal::ASTMatchFinder* Finder,
> ast_matchers::internal::BoundNodesTreeBuilder* Builder) const override {
> return llvm::any_of(Matchers, [&Node](const MatcherPair& Matcher) {
> if (Matcher.matchQualifiedName) {
> return Matcher.regex.match(Node.getQualifiedNameAsString());
> }
> return Matcher.regex.match(Node.getName());
> });
> }
>
> private:
> std::vector<MatcherPair> Matchers;
> };
>
> inline ::clang::ast_matchers::internal::Matcher<Type> matchesAnyListedName(
> llvm::ArrayRef<std::string> NameList) {
> return ::clang::ast_matchers::internal::makeMatcher(
> new MatchesAnyListedNameMatcher(NameList))
> }
> ```
+1 the idea looks good to me.
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
