[PATCH] D5238: [analyzer] Detect duplicate [super dealloc] calls

[Anna Zaks via cfe-commits]

zaks.anna added a comment.

Looks good, below are some comments which are mostly nits.

What's the plan for bringing this out of alpha? Is it pending evaluation on real code?


================
Comment at: lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp:11
@@ +10,3 @@
+// This defines ObjCSuperDeallocChecker, a builtin check that checks for the
+// correct use of the [super dealloc] message in -dealloc in MRR mode.
+//
----------------
Please, be more specific about the properties we are checking for.

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp:91
@@ +90,3 @@
+
+  if (SelfSymbol == SelfVal.getAsSymbol()) {
+    Satisfied = true;
----------------
Not sure if the extra check buys us much (right?), but it's not on a hot path, so we could keep it.

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp:102
@@ +101,3 @@
+    return new PathDiagnosticEventPiece(
+        L, "[super dealloc] originally called here");
+  }
----------------
maybe remove "originally"?

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp:112
@@ +111,3 @@
+  DoubleSuperDeallocBugType.reset(
+      new BugType(this, "[super dealloc] should never be called twice",
+                  categories::CoreFoundationObjectiveC));
----------------
"should never be" -> "should not be"?
"twice" -> "more than once"

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp:126
@@ +125,3 @@
+
+  State = State->add<CalledSuperDealloc>(SelfSymbol);
+  C.addTransition(State);
----------------
So we are actually storing the symbol that refers to 'super', right? The receiver of 'dealloc', not the 'self' of the object whose methods are being analyzed. This was confusing reading the code top down; for example, in the bug visitor.

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp:155
@@ +154,3 @@
+      new BugReport(*DoubleSuperDeallocBugType,
+                    "[super dealloc] called a second time", ErrNode));
+  BR->addRange(M.getOriginExpr()->getSourceRange());
----------------
Maybe we should say "more than once" or "again" in case we've missed a call to [super] dealloc.

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp:164
@@ +163,3 @@
+void
+ObjCSuperDeallocChecker::initIdentifierInfoAndSelectors(ASTContext &Ctx) const {
+  if (IIdealloc)
----------------
You should consider extending the helper method that Gabor added for checking if a specific function has been called in 
http://reviews.llvm.org/D15921

================
Comment at: test/Analysis/DeallocUseAfterFreeErrors.m:271
@@ +270,3 @@
+  [super dealloc]; // expected-note {{[super dealloc] originally called here}}
+  [super dealloc]; // expected-warning {{[super dealloc] called a second time}} expected-note {{[super dealloc] called a second time}}
+
----------------
you can put expected-note on the next line using "expected-note at -1"


http://reviews.llvm.org/D5238