Page MenuHomePhabricator

[ASTMatchers] Introduce a matcher for matching any given Objective-C selector
ClosedPublic

Authored by george.karpenkov on Mar 23 2018, 6:08 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

alexfh requested changes to this revision.Mar 26 2018, 9:06 AM
alexfh added inline comments.
include/clang/ASTMatchers/ASTMatchers.h
2737 ↗(On Diff #139688)

I'd suggest to do the same as with hasAnyName: leave just the std::vector<llvm::StringRef> overload. WDYT?

This revision now requires changes to proceed.Mar 26 2018, 9:06 AM
include/clang/ASTMatchers/ASTMatchers.h
2737 ↗(On Diff #139688)

OK.
I think it much more sense to accepts a vector of std::string in the first place, since we need an owning container for the matcher anyway.

george.karpenkov edited the summary of this revision. (Show Details)

@alexfh OK to commit?

Not yet. A few more comments.

lib/ASTMatchers/ASTMatchersInternal.cpp
327 ↗(On Diff #139856)

What's the point of this variable? Maybe just use its initializer directly?

334 ↗(On Diff #139856)

const std::string & / const auto & ?

335 ↗(On Diff #139856)

S == SelString?

342 ↗(On Diff #139856)

What's the point of this variable? Maybe just use its initializer directly?

416 ↗(On Diff #139856)

What was wrong with the partially qualified name here?

alexfh requested changes to this revision.Mar 28 2018, 11:34 AM
This revision now requires changes to proceed.Mar 28 2018, 11:34 AM
george.karpenkov marked 3 inline comments as done.
george.karpenkov added inline comments.
lib/ASTMatchers/ASTMatchersInternal.cpp
327 ↗(On Diff #139856)

The principle of not having too many things magically happening in one line would be IMO violated if we write internal::Matcher<NamedDecl>(new internal::HasNameMatcher(vectorFromRefs(NameRefs)))

416 ↗(On Diff #139856)

It does not work.
Using an AST_MATCHER_P macro in ASTMatchersInternal constructs a matcher inside the internal namespace, breaking the previous version of this function.

alexfh accepted this revision.Mar 28 2018, 3:21 PM

LG

lib/ASTMatchers/ASTMatchersInternal.cpp
327 ↗(On Diff #139856)

I wouldn't consider any of this stuff magical, but I also won't object to retaining this variable.

This revision is now accepted and ready to land.Mar 28 2018, 3:21 PM
This revision was automatically updated to reflect the committed changes.