[cfe-commits] [PATCH] update to clang analyzer ObjCMissingSuperCallChecker: check classes other than UIViewController too

[Jordan Rose]

  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
+ at interface TestD : UIResponder {}
----------------
Please test //all// of the new methods you are adding!


http://llvm-reviews.chandlerc.com/D129