[PATCH] D12445: [Static Analyzer] Remove sinks from nullability checks.

[Anna Zaks via cfe-commits]

zaks.anna added inline comments.

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:166
@@ +165,3 @@
+  // nullability precondition is violated it will not report any bugs at all.
+  void reportBug(ErrorKind Error, ExplodedNode *N, const MemRegion *Region,
+                 CheckerContext &C, const Stmt *ValueExpr = nullptr) const;
----------------
The difference is not clear by looking at the signature; maybe you should rename the method to make it more clear what the difference is.

Please, use doxygen style comments everywhere!

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:228
@@ -222,1 +227,3 @@
 
+// If the nullability preconditions of a function is violated, we should not
+// report that, a postcondition is violated. For this reason once a precondition
----------------
preconditions -> precondition
I am not sure what you mean by postcondition and where that term is defined.
turned of ->  turned off
so this check does not -> so that this checker would not lead to reduced coverage

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:371
@@ +370,3 @@
+      checkPreconditionViolation(OriginalState, C.getLocationContext());
+  if (NewState != OriginalState) {
+    C.addTransition(NewState, N);
----------------
Looks like you are assuming that you cannot get to this point with OriginalState having the PreconditionViolation bit set. This seems fragile. At minimum, you should assert this, but maybe changing the checkPreconditionViolation API would be better?

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:395
@@ +394,3 @@
+  // reaped.
+  if (!State->get<PreconditionViolated>()) {
+    ProgramStateRef NewState =
----------------
Maybe you should only do this if there was at least one symbol with nullability info removed in the loop above? Would that work?

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:430
@@ -338,1 +429,3 @@
+    // Do not suppress errors on defensive code paths, because dereferencing
+    // null is alwazs an error.
     if (Event.IsDirectDereference)
----------------
Please, spell check your comments.

We are not dereferencing null.. we are dereferencing a nullable pointer..
Am I wrong?

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:565
@@ -468,5 +564,3 @@
           ParamNullability == Nullability::Nonnull) {
-        static CheckerProgramPointTag Tag(this, "NullablePassedToNonnull");
-        ExplodedNode *N = C.generateSink(State, C.getPredecessor(), &Tag);
-        reportBug(ErrorKind::NullablePassedToNonnull, N, Region,
-                  C.getBugReporter(), ArgExpr);
+        State = State->set<PreconditionViolated>(true);
+        ExplodedNode *N = C.addTransition(State);
----------------
I do not understand how this works..

Does this ever report an issue? I thought this version of the bugReport will only report an issue if PreconditionViolation is not set. (Same for the code below and above..)

Is this tested?




http://reviews.llvm.org/D12445