================
Comment at: lib/StaticAnalyzer/Checkers/CMakeLists.txt:19
@@ -18,2 +18,3 @@
CastToStructChecker.cpp
+ CheckObjCViewController.cpp
CheckObjCDealloc.cpp
----------------
I think even though it only does UIViewController right now, it's okay to be
optimistic and call it "MissingSuperCall" and "ObjCMissingSuperCallChecker" or
something. Although since it's still in the "alpha" package it's not like the
name will be hardcoded anywhere yet.
================
Comment at: lib/StaticAnalyzer/Checkers/CheckViewController.cpp:1
@@ +1,2 @@
+//==- CheckObjCViewController.cpp - Check ObjC UIViewController subclass
implementation --*- C++ -*-==//
+//
----------------
Please respect the 80-column limit even in the file header. You can squeeze a
few more characters out of the line by removing the -*- c++ -*- note -- that's
only necessary for header files, where the editor can't tell from the extension
that it's C++ source. (It doesn't hurt, of course.)
================
Comment at: lib/StaticAnalyzer/Checkers/CheckViewController.cpp:11
@@ +10,3 @@
+// This file defines a CheckObjCViewController, a checker that
+// analyzes an UIViewController implementation to determine if it
+// correctly calls super in the methods where this is mandatory.
----------------
This is very nitpicky, but I've always heard people pronounce UIViewController
as "yu-ai-view-controller", and therefore the determiner should be "a", not
"an". ;-)
================
Comment at: lib/StaticAnalyzer/Checkers/CheckViewController.cpp:30
@@ +29,3 @@
+
+static bool scan_selector(Stmt *S, Selector selector) {
+ if (ObjCMessageExpr *ME = dyn_cast<ObjCMessageExpr>(S))
----------------
Please follow the LLVM naming conventions: the function should be named
scanSelector and the parameter name start with a capital letter (something like
"Sel"). I realize you grabbed this from dealloc checker, but that's no reason
we shouldn't follow the conventions going forwards!
Also, I'd personally prefer this to be a const Stmt *, since you have no
intention of modifying it.
================
Comment at: lib/StaticAnalyzer/Checkers/CheckViewController.cpp:33-38
@@ +32,8 @@
+ if (ME->getSelector() == selector) {
+ switch (ME->getReceiverKind()) {
+ case ObjCMessageExpr::Instance: return false;
+ case ObjCMessageExpr::SuperInstance: return true;
+ case ObjCMessageExpr::Class: break;
+ case ObjCMessageExpr::SuperClass: break;
+ }
+ }
----------------
For -dealloc this was okay, but some of your selectors take arguments, and this
may come back to hurt us if something like this becomes checkable in the future:
lang=objc
- (id)foo:(id)arg {
return [bar foo:[super foo:arg]];
}
I think you need to continue recursing even in the Instance case.
Stepping back a bit, I think you should be using RecursiveASTVisitor instead of
manually walking through the children of statements. The reason is that some
statements do odd things with their children (PseudoObjectExpr and
OpaqueValueExpr in particular, which are used to represent dot-syntax access to
properties.) In this case you will just need a very simple RAV that only visits
ObjCMessageExprs.
================
Comment at: test/Analysis/viewcontroller.m:3
@@ +2,3 @@
+
+//===--- BEGIN: Delta-debugging reduced headers.
--------------------------===//
+
----------------
These headers don't look delta-reduced, but that's okay. :-)
================
Comment at: test/Analysis/viewcontroller.m:33-46
@@ +32,16 @@
+@end
+/*@implementation UIViewController
+- (void)addChildViewController:(UIViewController *)childController {}
+- (void)viewDidAppear:(BOOL)animated {}
+- (void)viewDidDisappear:(BOOL)animated {}
+- (void)viewDidUnload {}
+- (void)viewDidLoad {}
+- (void)viewWillUnload {}
+- (void)viewWillAppear:(BOOL)animated {}
+- (void)viewWillDisappear:(BOOL)animated {}
+- (void)didReceiveMemoryWarning {}
+- (void)removeFromParentViewController {}
+- (id)retain { return 0; }
+- (oneway void)release {}
+@end*/
+
----------------
As you've noticed, there's no need to put the @implementation in the test; this
comment can just be removed.
================
Comment at: test/Analysis/viewcontroller.m:50
@@ +49,3 @@
+
+// dont warn if UIViewController isn't our superclass
+@interface TestA
----------------
Typo: "Don't". (Please do capitalize as well.)
================
Comment at: test/Analysis/viewcontroller.m:68
@@ +67,3 @@
+
+// warn if UIViewController isn't our superclass
+@interface TestB : UIViewController {}
----------------
I think you mean "is". Also, please include some more negative tests as well:
cases where the user overrode a method but correctly called super.
http://llvm-reviews.chandlerc.com/D68
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
