[PATCH] D17511: [analyzer] Make ObjCDeallocChecker path sensitive.

[Anna Zaks via cfe-commits]

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