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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
