This is an archive of the discontinued LLVM Phabricator instance.

[ASTMatchers] Add ObjC matchers for fundamental decls
ClosedPublic

Authored by kastiglione on Mar 10 2017, 4:59 PM.

Details

Summary

This adds matchers for ObjCProtocolDecl, ObjCCategoryDecl, ObjCMethodDecl, ObjCIvarDecl, and
ObjCPropertyDecl. These matchers complement the existing matcher for ObjCInterfaceDecl.

Event Timeline

kastiglione created this revision.Mar 10 2017, 4:59 PM

I'd like to add more ObjC specific matcher functionality in follow up diffs. This diff is to to start somewhere, and get feedback.

This revision is now accepted and ready to land.Mar 10 2017, 11:23 PM

Thanks @malcolm.parsons. I don't have commit access, will you be able to commit this?

aaron.ballman added inline comments.
unittests/ASTMatchers/ASTMatchersNodeTest.cpp
1501

These changes are unrelated and should be in a separate commit.

1546

Instead of using a pragma for this, I think it would make more sense to just modify matchesObjC() to disable the diagnostic. This is only intended to test the dynamic AST matchers, so the diagnostics are not useful in that case anyway.

unittests/ASTMatchers/ASTMatchersTest.h
135–138

Can you explain why this change is required?

kastiglione added inline comments.Mar 14 2017, 9:13 AM
unittests/ASTMatchers/ASTMatchersNodeTest.cpp
1501

They're related to the use of either -fobjc-runtime= or fobjc-nonfragile-abi.

1546

matchesConditionally() accepts only one compiler arg, so putting the diagnostics here was a smaller change than refactoring that function. Do you think it would be better to refactor matchesConditionally()?

unittests/ASTMatchers/ASTMatchersTest.h
135–138

Code was not being evaluated as Objective-C 2, which resulted in warnings and errors for the test this diff introduces. Setting the runtime was the first approach I tried, and it worked so I went with it without looking into why it was necessary. Now that you've asked, I stepped through and found that the i386-unknown-unknown triple is resulting in the use of an ELF toolchain and GCC objc runtime.

It can be changed to -fobjc-nonfragile-abi, which seems better than a specific runtime, do you agree? Is there any reason to not have Objective-C 2 be the default?

kastiglione added inline comments.Mar 14 2017, 9:43 AM
unittests/ASTMatchers/ASTMatchersNodeTest.cpp
1546

I notice that many other tests have warnings. Should these tests just allow the warnings to be emitted?

Use -fobjc-nonfragile-abi

aaron.ballman added inline comments.Mar 14 2017, 9:58 AM
unittests/ASTMatchers/ASTMatchersNodeTest.cpp
1546

We generally let the warnings go -- it's not harmful to have them. However, if this is a warning that's likely to trigger on most tests, there's no harm in suppressing it either.

unittests/ASTMatchers/ASTMatchersTest.h
135–138

I think -fobjc-nonfragile-abi may be fine, but I guess I'm surprised that ObjC1 didn't require any specific runtime and ObjC2 requires one or else you get errors (warnings are fine, however -- we have plenty of those in these tests).

Perhaps it's time to fix the FIXME in matchesConditionally() so that we don't need to specify the triple at all, and then you won't need to specify the runtime? I don't think that should hold up this patch, however.

kastiglione added inline comments.Mar 14 2017, 10:13 AM
unittests/ASTMatchers/ASTMatchersNodeTest.cpp
1546

Sounds good, I'm for suppressing them. Should I refactor matchesConditionally() to allow multiple compile args, and disable these warnings from matchesObjC()?

unittests/ASTMatchers/ASTMatchersTest.h
135–138

I'm surprised that ObjC1 didn't require any specific runtime and ObjC2 requires one or else you get errors

I think this is because the existing ObjC in this test was small in size and coverage of syntax/features. Given the variable name Objc1String, it was probably written to avoid ObjC2 specific abi/features.

aaron.ballman added inline comments.Mar 14 2017, 10:26 AM
unittests/ASTMatchers/ASTMatchersNodeTest.cpp
1546

Yes, I think that's the way to go.

unittests/ASTMatchers/ASTMatchersTest.h
135–138

Fair enough

Overload matchesConditionally() for multiple compiler args

aaron.ballman added inline comments.Mar 15 2017, 9:10 AM
unittests/ASTMatchers/ASTMatchersTest.h
64

I think this might be better as an llvm::ArrayRef<llvm::StringRef>.

115

This could then use makeArrayRef(CompileArg).

kastiglione added inline comments.Mar 15 2017, 10:45 AM
unittests/ASTMatchers/ASTMatchersTest.h
64

Ok. It needs to be a std::vector<std::string> for runToolOnCodeWithArgs(). I don't see any built in way to do that conversion, so this function will have to manually do that I guess?

aaron.ballman added inline comments.Mar 15 2017, 10:51 AM
unittests/ASTMatchers/ASTMatchersTest.h
64

Can you do:

std::vector<std::string> LocalArgs(Args.begin(), Args.end());
LocalArgs.insert(LocalArgs.end(), {"everything else"});

below?

Use ArrayRef<StringRef>

unittests/ASTMatchers/ASTMatchersTest.h
64

right, thanks!

aaron.ballman accepted this revision.Mar 15 2017, 12:36 PM

LGTM! I'll go ahead and commit for you.

unittests/ASTMatchers/ASTMatchersTest.h
114

The formatting of this line is incorrect, but I can fix it when I commit.

aaron.ballman closed this revision.Mar 15 2017, 1:26 PM

I've commit in r297882, thanks!

@aaron.ballman @malcolm.parsons Thanks for the reviews and the commit. I plan to do follow ups for more ObjC ASTMatcher support.