[PATCH] D11468: [Static Analyzer] The first implementation of nullability checker.

[Anna Zaks via cfe-commits]

zaks.anna added a comment.

Partial review...


================
Comment at: lib/StaticAnalyzer/Checkers/Checkers.td:137
@@ +136,3 @@
+def NullPassedToNonnullChecker : Checker<"NullPassedToNonnull">,
+  HelpText<"Warns when a null pointer is passed to a nonnull pointer.">,
+  DescFile<"NullabilityChecker.cpp">;
----------------
Should it be "pointer is used as a nonnull argument"?

================
Comment at: lib/StaticAnalyzer/Checkers/Checkers.td:149
@@ +148,3 @@
+def NullablePassedToNonnullChecker : Checker<"NullablePassedToNonnull">,
+  HelpText<"Warns when a nullable pointer is passed to a nonnull pointer.">,
+  DescFile<"NullabilityChecker.cpp">;
----------------
"nullable pointer is used as a nonnull argument"?

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:12
@@ +11,3 @@
+// checker is that, the user is running this checker after all the nullability
+// warnings that is emitted by the compiler was fixed.
+//
----------------
"is" -> "are"

How are we relying on this assumption? What happens if they are not fixed?

Also we should describe what we mean by nullability warnings, maybe refer to the annotations themselves here? It would be great to give a high level overflow of the algorithm and maybe even talk about the difference between the checks?

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:54
@@ +53,3 @@
+  return static_cast<Nullability>(
+      std::min(static_cast<char>(Lhs), static_cast<char>(Rhs)));
+}
----------------
??

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:70
@@ +69,3 @@
+const char *ErrorMessages[] = {
+    "Null pointer is assigned to nonnull pointer",
+    "Null pointer is passed to a nonnull parameter",
----------------
Same as above - there is no such thing as "nonnull pointer".

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:152
@@ +151,3 @@
+      R->addRange(ValueExpr->getSourceRange());
+      if (Error < ErrorKind::NullPointerEnd)
+        bugreporter::trackNullOrUndefValue(N, ValueExpr, *R);
----------------
I don't like that we assume ordering here. Someone might sort the values in the enum and break this. Please, change this.


================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:179
@@ +178,3 @@
+  Nullability Nullab;
+  const Stmt *Source;
+};
----------------
Please, add comments.
What is Source?

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:192
@@ +191,3 @@
+
+static bool shouldTrackRegion(const MemRegion *Region,
+                              AnalysisDeclContext *DeclContext) {
----------------
Is this used for optimization purposes? Can you describe the rules we use here?

What's the benefit of not tracking self?

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:259
@@ +258,3 @@
+
+static Nullability getNullability(QualType Type) {
+  const auto *AttrType = Type->getAs<AttributedType>();
----------------
Maybe rename to "getNullabilityAnnotation" ?

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:267
@@ +266,3 @@
+    return Nullability::Nonnull;
+  return Nullability::Unspecified;
+}
----------------
shouldn't this be an llvm_unreachable?

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:298
@@ +297,3 @@
+    TrackedNullability = State->get<NullabilityMap>(
+        Region->getAs<SubRegion>()->getSuperRegion());
+    if (!TrackedNullability)
----------------
Are we sure that if "!TrackedNullability", the event complains about a dereference on the parent (like field access)?

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:317
@@ +316,3 @@
+  QualType RetExprType = RetExpr->getType();
+  if (!RetExprType->isPointerType() && !RetExprType->isObjCObjectPointerType())
+    return;
----------------
Let's use an existing helper here:
inline bool Type::isAnyPointerType() const {
  return isPointerType() || isObjCObjectPointerType();
}


http://reviews.llvm.org/D11468