zaks.anna added inline comments.
================
Comment at: lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp:89
@@ -109,7 +88,3 @@
-static bool synthesizedPropertyRequiresRelease(const ObjCPropertyDecl *PD) {
- // A synthesized property must be released if and only if the kind of setter
- // was neither 'assign' or 'weak'.
- ObjCPropertyDecl::SetterKind SK = PD->getSetterKind();
- return (SK != ObjCPropertyDecl::Assign && SK != ObjCPropertyDecl::Weak);
-}
+//===----------------------------------------------------------------------===//
+// ObjCDeallocChecker
----------------
Let's not use these comments blocks. We should use oxygen style comments.
================
Comment at: lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp:161
@@ +160,3 @@
+
+// Diagnose when the class requires a -dealloc method and is missing one.
+void ObjCDeallocChecker::checkASTDecl(const ObjCImplementationDecl *D,
----------------
Use doxygen throughout. Please, mention that this is an AST check.
================
Comment at: lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp:184
@@ -144,21 +183,3 @@
- // Determine if the class subclasses NSObject.
- IdentifierInfo* NSObjectII = &Ctx.Idents.get("NSObject");
- IdentifierInfo* SenTestCaseII = &Ctx.Idents.get("SenTestCase");
-
- for ( ; ID ; ID = ID->getSuperClass()) {
- IdentifierInfo *II = ID->getIdentifier();
-
- if (II == NSObjectII)
- break;
-
- // FIXME: For now, ignore classes that subclass SenTestCase, as these don't
- // need to implement -dealloc. They implement tear down in another way,
- // which we should try and catch later.
- // http://llvm.org/bugs/show_bug.cgi?id=3187
- if (II == SenTestCaseII)
- return;
- }
-
- if (!ID)
+ if (isSuppressedClass(ID))
return;
----------------
Looks like we do not use this for the other checker. Is that the correct
behavior? Seems like it.
Maybe we should reflect this in the method name.
================
Comment at: lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp:198
@@ -179,2 +197,3 @@
if (!MD) { // No dealloc found.
+ const char* Name = "missing -dealloc";
----------------
We usually capitalize this.
================
Comment at: lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp:249
@@ +248,3 @@
+ // in that location has not changed.
+ State = State->bindLoc(LValLoc.getValue(), InitialVal,
+ /*notifyChanges*/false);
----------------
I do not understand this.
================
Comment at: lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp:261
@@ -255,4 +260,3 @@
-//===----------------------------------------------------------------------===//
-// ObjCDeallocChecker
-//===----------------------------------------------------------------------===//
+// If -dealloc is on the stack, handle the call if it is a release or a
+// nilling-out property setter.
----------------
I think it's worth adding "If we are in -dealloc or" ...
================
Comment at: lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp:298
@@ +297,3 @@
+// releases.
+void ObjCDeallocChecker::checkPostObjCMessage(
+ const ObjCMethodCall &M, CheckerContext &C) const {
----------------
Add: "We have to release before a call to [super dealloc]."
================
Comment at: lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp:383
@@ +382,3 @@
+ // at, for example, both a return and at the end of of the function.
+ State = State->remove<UnreleasedIvarValues>(IvarSymbol);
+
----------------
Hopefully, this ensures that all symbols added in the function begin call are
removed! The logic is slightly different than where we were adding these:
getContainingObjCImpl(LCtx)->property_impls()
================
Comment at: lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp:418
@@ +417,3 @@
+
+ OS << " by a synthesized property but was not released in 'dealloc'";
+
----------------
This might be too long.. Maybe we could remove "instance variable"?
================
Comment at: lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp:526
@@ +525,3 @@
+ MissingReleaseBugType.reset(
+ new BugType(this, "missing ivar release (leak)",
+ categories::MemoryCoreFoundationObjectiveC));
----------------
BugType names should be capitalized.
================
Comment at: lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp:721
@@ +720,3 @@
+/// Returns true if the \param ID is a class in which -dealloc warnings
+/// should be suppressed.
+bool ObjCDeallocChecker::isSuppressedClass(const ObjCInterfaceDecl *ID) const {
----------------
We might need an annotation for this as well.
http://reviews.llvm.org/D17511
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
