aaron.ballman added inline comments.
================ Comment at: unittests/ASTMatchers/ASTMatchersNodeTest.cpp:1547 + std::string ObjCString = + "#pragma clang diagnostic ignored \"-Wobjc-root-class\"\n" + "@protocol Proto " ---------------- kastiglione wrote: > aaron.ballman wrote: > > kastiglione wrote: > > > kastiglione wrote: > > > > aaron.ballman wrote: > > > > > 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. > > > > `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()`? > > > I notice that many other tests have warnings. Should these tests just > > > allow the warnings to be emitted? > > 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. > Sounds good, I'm for suppressing them. Should I refactor > `matchesConditionally()` to allow multiple compile args, and disable these > warnings from `matchesObjC()`? Yes, I think that's the way to go. ================ Comment at: unittests/ASTMatchers/ASTMatchersTest.h:123 Code, AMatcher, true, - "", FileContentMappings(), "input.m"); + "-fobjc-runtime=macosx", FileContentMappings(), "input.m"); } ---------------- kastiglione wrote: > aaron.ballman wrote: > > kastiglione wrote: > > > aaron.ballman wrote: > > > > Can you explain why this change is required? > > > `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? > > 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. > > 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. Fair enough https://reviews.llvm.org/D30854 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits