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

[Jordan Rose]

  Looking pretty good! Mostly just stylistic comments this time.


================
Comment at: lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp:92-93
@@ +91,4 @@
+///
+/// @param ObjCImplementationDecl The declaration to check for superclasses.
+/// @param SuperclassName [out] On return, the found superclass name.
+bool ObjCSuperCallChecker::isCheckableClass(const ObjCImplementationDecl *D,
----------------
Please use our Doxygen conventions: `\param ObjCImplementationDecl` and `\param[out] SuperclassName`.

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp:74-77
@@ +73,6 @@
+private:
+  void fillSelectorsOLD(ASTContext &Ctx, const size_t Count,
+                                         const char **SelectorNames,
+                                         const unsigned *ArgumentCounts,
+                                         const char *ClassName) const;
+  bool isCheckableClass(const ObjCImplementationDecl *D,
----------------
What is this doing here?

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp:109
@@ +108,3 @@
+                                         const char *ClassName) const { 
+  llvm::SmallSet<Selector, 16> ClassSelectors;
+  // Fill the Selectors SmallSet with all selectors we want to check.
----------------
This is going to cause us to need a copy at the end. It's probably better to do this:

  llvm::SmallSet<Selector, 16> &ClassSelectors = SelectorsForClass[ClassName];

which will default-construct the set in-place.

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp:82
@@ +81,3 @@
+  void fillSelectors(ASTContext &Ctx, llvm::ArrayRef<SelectorDescriptor> Sel,
+                     const char *ClassName) const;
+  mutable llvm::StringMap<llvm::SmallSet<Selector, 16> > SelectorsForClass;
----------------
Might as well use StringRef here too.

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp:79
@@ +78,3 @@
+  bool isCheckableClass(const ObjCImplementationDecl *D,
+                        llvm::StringRef &SuperclassName) const;
+  void initializeSelectors(ASTContext &Ctx) const;
----------------
StringRef is one of the LLVM types that's so common we imported it into the `clang` namespace as well, so it doesn't need the LLVM prefix. (I think I said this before; sorry.)

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp:81
@@ +80,3 @@
+  void initializeSelectors(ASTContext &Ctx) const;
+  void fillSelectors(ASTContext &Ctx, llvm::ArrayRef<SelectorDescriptor> Sel,
+                     const char *ClassName) const;
----------------
ArrayRef also does not need a prefix any more.

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp:114
@@ +113,3 @@
+    SelectorDescriptor Descriptor = *I;
+	assert(Descriptor.ArgumentCount <= 1); // No multi-argument selectors yet.
+
----------------
Funky indentation here.

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp:213
@@ -132,3 +212,3 @@
         const char *Name = "Missing call to superclass";
-        SmallString<256> Buf;
+        SmallString<320> Buf;
         llvm::raw_svector_ostream os(Buf);
----------------
Did you measure this? Is that why it grew?


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