[PATCH] D35613: Add Support for Generic Reference Counting Annotations in RetainCountChecker

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin added a comment.

This looks pretty good. There are some minor comments in line.

One thing that is missing: tests for the new diagnostic text. Can you add new tests that specifically test for the new diagnostic text with // expected-warning {{ ... }}}. These should probably go in retain-release.m.



================
Comment at: include/clang/StaticAnalyzer/Checkers/ObjCRetainCount.h:150
+    /// Indicates that the tracked object is a generic C object.
+    GenericC
   };
----------------
I would suggest calling this just "Generalized" to avoid confusion with generics and because it might not be C (it could, for example, be C++).


================
Comment at: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp:1343
     return RetEffect::MakeOwned(RetEffect::CF);
+  else if (hasRCAnnotation(D, "give"))
+    return RetEffect::MakeOwned(RetEffect::GenericC);
----------------
I would suggest having the annotation be "rc_ownership_returns_retained". This is consistent  with the CF and NS attribute names. I wouldn't be too concerned about this being verbose since users of the annotation in ISL will still use "__give".


================
Comment at: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp:1368
       Template->addArg(AF, parm_idx, DecRefMsg);
-    else if (pd->hasAttr<CFConsumedAttr>())
+    else if (pd->hasAttr<CFConsumedAttr>() || hasRCAnnotation(pd, "take"))
       Template->addArg(AF, parm_idx, DecRef);
----------------
I would suggest having the annotation be "rc_ownership_consumed".


Repository:
  rL LLVM

https://reviews.llvm.org/D35613