dcoughlin added a comment.
Thanks for the tests.
Have you tried this on the ISL codebase to make sure it is suppressing the
diagnostics in related to reference counting implementation that you expect?
I think it would be good to add some tests that reflect the reference counting
implementation patterns in ISL that you want to make sure not to warn on. Can
you simplify those patterns to their essence and add them as tests?
================
Comment at: test/Analysis/retain-release-inline.m:306
+void test2() {
+ NSString *s = [[NSString alloc] init];
+ foo(s);
----------------
What is the purpose of this test? Does it test new functionality added by your
patch?
================
Comment at: test/Analysis/retain-release-inline.m:315
+
+__attribute__((annotate("rc_ownership_trusted_implementation"))) void
test2_with_trusted_implementation_annotate_attribute() {
+ NSString *s = [[NSString alloc] init];
----------------
Can you simplify this test so that it directly tests the new functionality you
added? In general we try to avoid duplicating unnecessary parts of other tests.
This slows down running the tests and also adds a maintenance burden when we
need to change the test.
================
Comment at: test/Analysis/retain-release-inline.m:316
+__attribute__((annotate("rc_ownership_trusted_implementation"))) void
test2_with_trusted_implementation_annotate_attribute() {
+ NSString *s = [[NSString alloc] init];
+ foo(s);
----------------
I think it would be better to use C here rather than Objective-C. Can you
change the test to use new prototypes for the specific purpose of this test.
One that is annotated "__attribute__((cf_returns_retained))" (this is similar
to __isl_give) and one with a parameter that is that is annotated
"__attribute__((cf_consumed))".
The reason it doesn't make sense to use Objective-C here because we want this
new annotation to be used for C frameworks with their own reference counting
implementation -- and Objective-C's is built into the language.
Repository:
rL LLVM
https://reviews.llvm.org/D34937
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
