[PATCH] D34937: Suppressing Reference Counting Diagnostics for Functions Containing 'rc_ownership_trusted_implementation' Annotate Attribute

[Devin Coughlin via Phabricator via cfe-commits]

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