kastiglione added inline comments.

================
Comment at: unittests/ASTMatchers/ASTMatchersNodeTest.cpp:1501
 
-  std::string Objc1String =
+  std::string ObjCString =
+    "#pragma clang diagnostic ignored \"-Wobjc-root-class\"\n"
----------------
aaron.ballman wrote:
> These changes are unrelated and should be in a separate commit.
They're related to the use of either `-fobjc-runtime=` or 
`fobjc-nonfragile-abi`.


================
Comment at: unittests/ASTMatchers/ASTMatchersNodeTest.cpp:1547
+  std::string ObjCString =
+    "#pragma clang diagnostic ignored \"-Wobjc-root-class\"\n"
+    "@protocol Proto "
----------------
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()`?


================
Comment at: unittests/ASTMatchers/ASTMatchersTest.h:123
     Code, AMatcher, true,
-    "", FileContentMappings(), "input.m");
+    "-fobjc-runtime=macosx", FileContentMappings(), "input.m");
 }
----------------
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?


https://reviews.llvm.org/D30854



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to