[cfe-commits] [PATCH] new clang-analyzer checker that verifies ObjC API usage in UIViewController subclasses [v3]
Jordan Rose
jordan_rose at apple.com
Thu Oct 25 10:11:37 PDT 2012
Good job with the revision. By the way, you should be able to update an existing patch; you don't have to submit a new patch every time.
================
Comment at: lib/StaticAnalyzer/Checkers/CMakeLists.txt:19
@@ -18,2 +18,3 @@
CastToStructChecker.cpp
+ CheckObjCViewController.cpp
CheckObjCDealloc.cpp
----------------
Don't forget to change this to match the new source file name!
================
Comment at: lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp:33
@@ +32,3 @@
+ : public RecursiveASTVisitor<FindSupercallVisitor> {
+ public:
+
----------------
The convention is not to indent this past 'class'.
================
Comment at: lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp:35-38
@@ +34,6 @@
+
+ explicit FindSupercallVisitor(ASTContext *Context, Selector Sel)
+ : DoesCallSuper(0)
+ ,Context(Context)
+ , Sel(Sel) {}
+
----------------
I think the convention is to put all the initialization on one line, though we could use an official coding style for it. Also, it looks like you don't actually use the ASTContext at all.
================
Comment at: lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp:46
@@ +45,3 @@
+ }
+ return !DoesCallSuper; // Recurse if we didn't find the super call yet.
+ }
----------------
Nitpicking: please put the comment on the line before the return.
(But on the plus side, this is a well-formed LLVM inline comment: a full sentence beginning with a capital letter.)
================
Comment at: lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp:71-72
@@ +70,4 @@
+
+ if (II == NSObjectII)
+ break;
+
----------------
I would actually just not bother to test this. When you get to the root class, `ID->getSuperClass()` will be null and the loop will end.
================
Comment at: lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp:81-82
@@ +80,4 @@
+
+ if (!ID)
+ return;
+
----------------
Not sure why this is here.
================
Comment at: lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp:84-90
@@ +83,9 @@
+
+ const int selectorCount = 10;
+ const char *selectorNames[selectorCount] =
+ {"addChildViewController", "viewDidAppear", "viewDidDisappear",
+ "viewWillAppear", "viewWillDisappear", "removeFromParentViewController",
+ "didReceiveMemoryWarning", "viewDidUnload", "viewWillUnload", "viewDidLoad"};
+ const int selectorArgumentCounts[selectorCount] =
+ {1, 1, 1, 1, 1, 0, 0, 0, 0, 0};
+
----------------
Sorry I didn't mention this the first time:
a) Please use LLVM naming conventions even for local variables. We capitalize all our nouns now, like German. ;-)
b) I'm not sure about having a separate 'SelectorCount' variable. Explicitly setting the size of both arrays does ensure that they're the same size, but it also masks a potential issue if one array is too short. I think I'd prefer having the arrays be auto-sized and asserting that they are the same length on the next line. (LLVM has an nice array_lengthof function that can do this in LLVM/Support/STLExtras.h)
c) Since the `NumArgs` argument for SelectorTable::getSelector is typed as `unsigned`, 'SelectorArgumentCounts' should probably be an array of `unsigned` as well (along with the loop variable below).
================
Comment at: lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp:103-110
@@ +102,10 @@
+
+ // Scan the instance methods for the selector.
+ for (ObjCImplementationDecl::instmeth_iterator I = D->instmeth_begin(),
+ E = D->instmeth_end(); I!=E; ++I) {
+ if ((*I)->getSelector() == S) {
+ MD = *I;
+ break;
+ }
+ }
+
----------------
This loops over all the methods for every selector. Maybe you could build a set of selectors first (see LLVM's SmallSet), then loop over the methods and see if any of them have selectors in the set.
================
Comment at: lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp:118
@@ +117,3 @@
+ PathDiagnosticLocation DLoc =
+ PathDiagnosticLocation::createBegin(MD, BR.getSourceManager());
+
----------------
Nitpicking: please indent this to show it's a continuation of the previous line. Also, for consistency with the compiler warning for a missing -dealloc, I think this should use `createEnd` instead of `createBegin`.
================
Comment at: lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp:120
@@ +119,3 @@
+
+ const char* name = "missing call to superclass";
+
----------------
Please capitalize ("Missing call to super") and please put the asterisk with the variable name, not the type.
================
Comment at: lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp:122-123
@@ +121,4 @@
+
+ std::string buf;
+ llvm::raw_string_ostream os(buf);
+ os << "The '" << S.getAsString()
----------------
Since we roughly know how long the message will be, we can allocate storage on the stack for it using LLVM's SmallString.
SmallString<CHARS> Buf;
llvm::raw_svector_ostream os(Buf);
================
Comment at: lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp:124-128
@@ +123,7 @@
+ llvm::raw_string_ostream os(buf);
+ os << "The '" << S.getAsString()
+ << "' instance method in UIViewController subclass '" << *D
+ << "' does not send a '" << S.getAsString()
+ << "' message to its super class (missing [super "
+ << S.getAsString() << "])";
+
----------------
Call me a pedant, but I don't like saying "a '-addChildViewController:'". How about simplifying to something like "The '-foo:' instance method in UIViewController subclass 'MyViewController' is missing a [super foo:] call"?
================
Comment at: lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp:147
@@ +146,3 @@
+ BugReporter &BR) const {
+ checkObjCSuperCall(cast<ObjCImplementationDecl>(D), mgr.getLangOpts(), BR);
+ }
----------------
You're not using the LangOpts argument, and the decl is already an ObjCImplementationDecl. Why not move the whole check into this method anyway? (Though you should probably still just declare it in the class, then define it out-of-line below.)
================
Comment at: lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp:165-166
@@ +164,4 @@
+*** trivial cases:
+UIView subclasses
++ initWithFrame
+
----------------
We probably won't ever want to do this for init methods -- people do funny things with designated initializers.
================
Comment at: lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp:168-169
@@ +167,4 @@
+
+UIResponder subclasses
++ resignFirstResponder
+
----------------
Please do not use + to indicate "should always call super". In Objective-C, + means "class method"!
================
Comment at: lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp:181
@@ +180,3 @@
+UIViewController subclasses
+- loadView (minus indicates it should never call super)
++ transitionFromViewController:toViewController:
----------------
Interesting. This is definitely a valuable check because it's the sort of thing that's easy to do by mistake.
http://llvm-reviews.chandlerc.com/D78
More information about the cfe-commits
mailing list