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
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
