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