Page MenuHomePhabricator

[ASTMatcher] Extend hasAnyArgument to ObjCMessageExpr
ClosedPublic

Authored by george.karpenkov on Mar 6 2018, 10:58 AM.

Details

Summary

Currently hasArgument works with both ObjC messages and function calls, but not hasAnyArgument.
This patch fixes that discrepancy, as it's often more convenient to use hasAnyArgument.

On a more general note, it would be great to have a common superclass for objc-call and function call, and a matcher matching that, but that's probably a job for another commit.

Diff Detail

Repository
rL LLVM

Event Timeline

You should also regenerate the documentation by running docs\tools\dump_ast_matchers.py.

include/clang/ASTMatchers/ASTMatchers.h
3419 ↗(On Diff #137237)

I think it would be beneficial to add an example to the documentation.

unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
412 ↗(On Diff #137237)

I'm not particularly keen on the raw string literal here. We don't use it all that often, but if you have strong feelings in favor of using them, we seem to use R"( rather than R"<<<(.

include/clang/ASTMatchers/ASTMatchers.h
3419 ↗(On Diff #137237)

There are tests, there is no example for C++ either, and ObjectiveC examples tend to be fairly verbose.
There is also no example for hasArgument.

unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
412 ↗(On Diff #137237)

Why not? Having ( forces you to care about brackets inside the string, and in this case it does have brackets.
Forcing everything inside a single line would seriously hamper readability.

aaron.ballman added inline comments.Mar 6 2018, 3:50 PM
include/clang/ASTMatchers/ASTMatchers.h
3414–3415 ↗(On Diff #137237)

Comment is now out of date.

3419 ↗(On Diff #137237)

Most AST matcher users are more familiar with C++ than ObjC. Having some example that shows what will and won't be matched is useful for them.

If it's too verbose to make an example, that's a different matter, but it seems like the example would only need to be: [foo bar:12 baz:bing]; level of exposition.

unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
412 ↗(On Diff #137237)

Some editors still struggle to understand it's a string literal. Nothing requires this to be a single line, as string literal concatenation is perfectly reasonable (and used elsewhere). See TEST(SubstTemplateTypeParmType, HasReplacementType) as an example.

include/clang/ASTMatchers/ASTMatchers.h
3419 ↗(On Diff #137237)

OK

unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
412 ↗(On Diff #137237)

@aaron.ballman OK I'll change it, but in general I don't think we should use an argument "there exist an editor not supporting this feature" to enforce a certain coding style.

aaron.ballman added inline comments.Mar 6 2018, 4:00 PM
unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
412 ↗(On Diff #137237)

Fair.

We may want to consider updating the LLVM Coding Standard about use of raw string literals. There's no mention one way or the other and we usually avoid things not explicitly listed as known to be well-supported, but the list of what is supported looks rather out of date. (Not suggesting you should do anything, more just an observation based on my own curiosity with the style guide.)

george.karpenkov marked 4 inline comments as done.

@aaron.ballman I think this version is OK?

aaron.ballman accepted this revision.Mar 6 2018, 5:30 PM

LGTM with a small documentation nit (note, the docs will need to be regenerated as well).

include/clang/ASTMatchers/ASTMatchers.h
3429 ↗(On Diff #137295)

I believe there's a missing semicolon on the message send.

This revision is now accepted and ready to land.Mar 6 2018, 5:30 PM

You should also regenerate the documentation by running docs\tools\dump_ast_matchers.py.

Unfortunately for me it dies with:

Probing http://clang.llvm.org/doxygen/classclang_1_1CUDAKernelCallExpr.html...
*** Unparsable: " } // namespace ast_matchers } // namespace clang " ***

I don't think it's from my changes: with them removed it dies as well.
Should this really be done manually by the programmer? Do we have a buildbot checking for those? Wouldn't it make more sense for the buildbot to upload the documentation directly?
IMO the repo should not contain build byproducts.

unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
412 ↗(On Diff #137237)

E.g. analyzer code for dumping HTML and CSS looked rather horrendous before raw strings (though that might be also used as an argument to have files in the toolchain rather then in the compiler...) I tend to quite like them for those cases.

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.