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

[Gábor Horváth via cfe-commits]

xazax.hun marked 11 inline comments as done.
xazax.hun added a comment.

Thank you for the review. I will upload the new version of the patch once the rest of it was reviewed.


================
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.
+//
----------------
zaks.anna wrote:
> "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?
There are no assumptions like that in the code of the checker right now. I updated the comment to reflect that.

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:54
@@ +53,3 @@
+  return static_cast<Nullability>(
+      std::min(static_cast<char>(Lhs), static_cast<char>(Rhs)));
+}
----------------
zaks.anna wrote:
> ??
Added a comment to clarify what it is and how it is used.

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:192
@@ +191,3 @@
+
+static bool shouldTrackRegion(const MemRegion *Region,
+                              AnalysisDeclContext *DeclContext) {
----------------
zaks.anna wrote:
> Is this used for optimization purposes? Can you describe the rules we use here?
> 
> What's the benefit of not tracking self?
Initially it was not just an optimization. It was also intended to deal with the caching out issue, but eventually it turned out that caching out was ok, and not an error in the checker. I did not erase the code afterwards.

There are several possibilities now:
* Leave this part as it is.
* Only track symbolic regions.
* Delete this function.

Which one do you prefer?


================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:267
@@ +266,3 @@
+    return Nullability::Nonnull;
+  return Nullability::Unspecified;
+}
----------------
zaks.anna wrote:
> shouldn't this be an llvm_unreachable?
There are several AttrKind.
Source: http://clang.llvm.org/doxygen/classclang_1_1AttributedType.html#ab4901f7a37f5539698cb5ebd706245ed

So that code is not unreachable.

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:298
@@ +297,3 @@
+    TrackedNullability = State->get<NullabilityMap>(
+        Region->getAs<SubRegion>()->getSuperRegion());
+    if (!TrackedNullability)
----------------
zaks.anna wrote:
> Are we sure that if "!TrackedNullability", the event complains about a dereference on the parent (like field access)?
Sometimes when there is an expression like p[0] or p->field, the event contains the region for the element or the field region. However the tracked information is only available for p. For this reason I also check whether the super region has some tracked nullability and complain about that region in this case.


http://reviews.llvm.org/D11468