kastiglione added inline comments.
================
Comment at: unittests/ASTMatchers/ASTMatchersNodeTest.cpp:1547
+ std::string ObjCString =
+ "#pragma clang diagnostic ignored \"-Wobjc-root-class\"\n"
+ "@protocol Proto "
----------------
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()`?
================
Comment at: unittests/ASTMatchers/ASTMatchersTest.h:123
Code, AMatcher, true,
- "", FileContentMappings(), "input.m");
+ "-fobjc-runtime=macosx", FileContentMappings(), "input.m");
}
----------------
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.
https://reviews.llvm.org/D30854
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
