Sorry for the delay. This seems like a good start; lots of little things to
fix, but it'll be good to get the new checks.
================
Comment at: lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp:65
@@ -73,1 +64,3 @@
public:
+ ObjCSuperCallChecker() : IsInitialized(0) {}
+
----------------
We tend to use 'false' for booleans, even during initialization.
================
Comment at: lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp:76
@@ +75,3 @@
+ const char *ClassName) const;
+ mutable std::map<std::string,llvm::SmallSet<Selector, 16> >
SelectorsForClass;
+ mutable bool IsInitialized;
----------------
This should probably be an `llvm::StringMap`, which is a bit more efficient
than `std::map`.
================
Comment at: lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp:83
@@ +82,3 @@
+bool ObjCSuperCallChecker::isCheckableClass(const ObjCImplementationDecl *D,
+ std::string &SuperclassName) const
{
+ const ObjCInterfaceDecl *ID = D->getClassInterface();
----------------
Ah, `SuperclassName` is an out parameter! It would be nice to have a Doxygen
comment for this function, because that's not immediately obvious.
Also, since class names are long-lived, you could use an `llvm::StringRef &`
instead and avoid all the copies. Even if you didn't take this route I'd still
suggest not calling `str()` until you found the class you were looking for.
(Which you can do now that you'll be using `StringMap`.)
================
Comment at: lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp:110
@@ +109,3 @@
+
+#define len(x) llvm::array_lengthof((x))
+
----------------
Sean Silva wrote:
> this is kind of sketchy...
I agree. If you want to use `using llvm::array_lengthof`, that's okay, but
that's as far as you should go. Assertions can be two lines and they'll still
print okay.
================
Comment at: lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp:105
@@ +104,3 @@
+ IdentifierInfo *II = &Ctx.Idents.get(SelectorCString);
+ Selector Sel = Ctx.Selectors.getSelector(ArgumentCount, &II);
+ SelectorsForClass[ClassName].insert(Sel);
----------------
Please assert that `ArgumentCount` is either 0 or 1 first, since this won't
work with multi-argument selectors yet.
================
Comment at: lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp:106
@@ +105,3 @@
+ Selector Sel = Ctx.Selectors.getSelector(ArgumentCount, &II);
+ SelectorsForClass[ClassName].insert(Sel);
+ }
----------------
It's a small thing, but you can avoid the lookup every time by assigning the
`SmallSet` to a reference outside the loop.
================
Comment at: lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp:113
@@ +112,3 @@
+void ObjCSuperCallChecker::initializeSelectors(ASTContext &Ctx) const {
+ {
+ const char *SelectorNames[] = {"addChildViewController", "viewDidAppear",
----------------
It would be nice to have comments at the top of each section here that say
which class it's initializing. If you don't want the comment to get out of date
with the code (from copypasta), you could make a new variable just for the
class name, and use that in the call to `fillSelectors`.
================
Comment at: lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp:114-120
@@ +113,9 @@
+ {
+ const char *SelectorNames[] = {"addChildViewController", "viewDidAppear",
+ "viewDidDisappear", "viewWillAppear", "viewWillDisappear",
+ "removeFromParentViewController", "didReceiveMemoryWarning",
+ "viewDidUnload", "viewWillUnload", "viewDidLoad",
"updateViewConstraints",
+ "encodeRestorableStateWithCoder", "restoreStateWithCoder"};
+ const unsigned SelectorArgumentCounts[] =
+ {1, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 1, 1};
+ assert(len(SelectorArgumentCounts) == len(SelectorNames));
----------------
I'm starting to think it makes more sense to just use a custom struct type for
this:
```const SelectorDescriptor *Selectors[] = {
{ "addChildViewController", 1 },
{ "viewDidAppear", 1 },
...
}```
It would make things a lot simpler, and allow you to just pass an
`llvm::ArrayRef<SelectorDescriptor>` to `fillSelectors`.
(If you can come up with a better name than "SelectorDescriptor", please do.)
================
Comment at: test/Analysis/superclass.m:144
@@ +143,3 @@
+
+// Do warn for UIResponder subclasses
+@interface TestD : UIResponder {}
----------------
Please test //all// of the new methods you are adding!
http://llvm-reviews.chandlerc.com/D129
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
