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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits