Page MenuHomePhabricator

[ASTMatchers] Adds a matcher called `hasAnyOperatorName`
ClosedPublic

Authored by njames93 on Feb 24 2020, 4:48 AM.

Details

Summary

Acts on BinaryOperator and UnaryOperator and functions the same as anyOf(hasOperatorName(...), hasOperatorName(...), ...)

Documentation generation isn't perfect but I feel that the python doc script needs updating for that

Diff Detail

Event Timeline

njames93 created this revision.Feb 24 2020, 4:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 24 2020, 4:48 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
njames93 edited the summary of this revision. (Show Details)Feb 24 2020, 4:50 AM
njames93 added reviewers: aaron.ballman, gribozavr2.
gribozavr2 accepted this revision.Feb 24 2020, 5:33 AM
gribozavr2 added inline comments.
clang/include/clang/ASTMatchers/ASTMatchersInternal.h
1870

Consider enhancing the message to say what the expected types are.

This revision is now accepted and ready to land.Feb 24 2020, 5:33 AM
aaron.ballman accepted this revision.Feb 24 2020, 5:38 AM

LGTM

clang/include/clang/ASTMatchers/ASTMatchersInternal.h
1880–1884

Can we use llvm::find() instead of a manual loop?

gribozavr2 added inline comments.Feb 24 2020, 5:39 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
4772

Any specific things you would like to improve? The fixme is rather vague.

njames93 updated this revision to Diff 246190.Feb 24 2020, 5:51 AM
njames93 marked an inline comment as done.
njames93 edited the summary of this revision. (Show Details)
  • Address nits
njames93 marked an inline comment as done.Feb 24 2020, 5:51 AM
njames93 added inline comments.
clang/include/clang/ASTMatchers/ASTMatchersInternal.h
1870

This is the reason c++17 removed the need for static_assert messages, the message isn't needed as you can figure the error from the predicate

1880–1884

llvm::is_contained would be better, but they involve creating temporaries, llvm::any_of does it best I think

njames93 marked an inline comment as done.Feb 24 2020, 6:06 AM
njames93 added inline comments.
clang/include/clang/ASTMatchers/ASTMatchers.h
4772

This is what comes up in the ASTMatchersReference docs.

Matcher<internal::HasOpNameMatcher>	hasAnyOperatorName	StringRef, ..., StringRef

I would like it to have

Matcher<BinaryOperator>  hasAnyOperatorName	StringRef, ..., StringRef
...
Matcher<UnaryOperator>  hasAnyOperatorName	StringRef, ..., StringRef

However how VariadicFunction is matched in the dumper won't support that yet. And If you want to support it properly regex wouldn't work, instead you'd need a parser

njames93 updated this revision to Diff 246203.Feb 24 2020, 7:26 AM
  • Improved docs
njames93 requested review of this revision.Feb 24 2020, 7:26 AM
njames93 marked 3 inline comments as done.

I have fixed up the docs to be more in line with other matchers

njames93 updated this revision to Diff 246335.Feb 24 2020, 4:11 PM
  • Rebase with trunk
gribozavr2 accepted this revision.Feb 24 2020, 11:25 PM
This revision is now accepted and ready to land.Feb 24 2020, 11:25 PM
This revision was automatically updated to reflect the committed changes.