[PATCH] D12381: [Static Analyzer] Merge the Objective-C Generics Checker into Dynamic Type Propagation checker.

[Anna Zaks via cfe-commits]

zaks.anna added a comment.

Partial review in-line.


================
Comment at: lib/StaticAnalyzer/Checkers/Checkers.td:456
@@ -455,2 +455,3 @@
 def ObjCGenericsChecker : Checker<"ObjCGenerics">,
   HelpText<"Check for incorrect usages of parameterized types.">,
+  DescFile<"DynamicTypePropagation.cpp">;
----------------
We need a better description here this one is too vague. Maybe something like this one:
"Check for type errors when using Objective-C generics."

================
Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:17
@@ -11,1 +16,3 @@
 //
+// Generics Checker:
+// This checker tries to find type errors that the compiler is not able to catch
----------------
Mention ObjC here.

================
Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:45
@@ +44,3 @@
+class DynamicTypePropagation
+    : public Checker<check::PreCall, check::PostCall, check::PreObjCMessage,
+                     check::PostObjCMessage, check::DeadSymbols,
----------------
nit: Let's go back to the formatting with a single line per callback.

================
Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:48
@@ +47,3 @@
+                     check::PostStmt<CastExpr>, check::PostStmt<CXXNewExpr>> {
+  mutable std::unique_ptr<BugType> BT;
+
----------------
Better name please, we might add more bug types.

================
Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:76
@@ +75,3 @@
+
+  void reportGenericsBug(const ObjCObjectPointerType *From,
+                         const ObjCObjectPointerType *To, ExplodedNode *N,
----------------
Please, move the definitions outside to reduce clutter.

================
Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:81
@@ +80,3 @@
+    initBugType();
+    SmallString<64> Buf;
+    llvm::raw_svector_ostream OS(Buf);
----------------
How do we know that the string is big enough?

================
Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:83
@@ +82,3 @@
+    llvm::raw_svector_ostream OS(Buf);
+    OS << "Incompatible pointer types assigning to '";
+    QualType::print(To, Qualifiers(), OS, C.getLangOpts(), llvm::Twine());
----------------
The error message does not sounds like a proper sentence..

Assigning from 'A' to 'B' sounds more natural.

Are we only warning on assignments? Maybe converting would be more applicable in some contexts..

Something like this might be better: "Conversion from value of type 'Integer' to incompatible type 'String'"

================
Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:105
@@ -43,1 +104,3 @@
+
+  DefaultBool CheckGenerics;
 };
----------------
Doxygen comment please.

================
Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:179
@@ -178,3 @@
-  // We only track dynamic type info for regions.
-  const MemRegion *ToR = C.getSVal(CastE).getAsRegion();
-  if (!ToR)
----------------
This line used to be unconditional and now, it's only executed if we are casting between ObjC Types.

================
Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:407
@@ +406,3 @@
+// Clean up the states stored by the generics checker.
+void DynamicTypePropagation::checkDeadSymbols(SymbolReaper &SR,
+                                              CheckerContext &C) const {
----------------
Do you know if the info tracked by the DynamicTypeInfo checker gets cleaned up for dead symbols?

================
Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:448
@@ -251,9 +447,3 @@
 
-  // Get the old and new types.
-  const ObjCObjectPointerType *NewTy =
-      CastE->getType()->getAs<ObjCObjectPointerType>();
-  if (!NewTy)
-    return nullptr;
-  QualType OldDTy = C.getState()->getDynamicTypeInfo(ToR).getType();
-  if (OldDTy.isNull()) {
-    return NewTy;
+/// Get the most derived class if From that do not loose information about type
+/// parameters. To has to be a subclass of From. From has to be specialized.
----------------
Please, use doxygen.
Also, the comment is not clear.. Ex: "most derived class if From"

================
Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:456
@@ +455,3 @@
+
+static bool storeWhenMoreInformative(ProgramStateRef &State, SymbolRef Sym,
+                                     const ObjCObjectPointerType *const *Old,
----------------
There s nothing about picking more informative type here.. especially with respect to informative term used in the previous function..


http://reviews.llvm.org/D12381