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