[cfe-commits] [PATCH] new clang-analyzer checker that verifies ObjC API usage in UIViewController subclasses [v2]

[Jordan Rose]

================
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 @@
+ at 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 {}
+ at 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
+ at 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
+ at 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