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

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

xazax.hun added inline comments.

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:395
@@ +394,3 @@
+  // reaped.
+  if (!State->get<PreconditionViolated>()) {
+    ProgramStateRef NewState =
----------------
zaks.anna wrote:
> Maybe you should only do this if there was at least one symbol with nullability info removed in the loop above? Would that work?
That would not work.
Only "nullable" nullability and contradictions are tracked for the symbols right now. If nothing is marked nullable, this branch would never be taken.

================
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)
----------------
zaks.anna wrote:
> Please, spell check your comments.
> 
> We are not dereferencing null.. we are dereferencing a nullable pointer..
> Am I wrong?
You are right.

================
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);
----------------
zaks.anna wrote:
> 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?
> 
> 
Yes, it works.

The reason is that, reportBug does report bugs, when PreconditionViolated is set to true. It does not report bugs, when one of the nonnull argument is constrained to be null. PreconditionViolated is used at the beginning of some callbacks to return early.

However I think this behavior might be confusing, so I refactored the code. From now on reportBugConditionally does not report any bug, when PreconditionViolated is set to true, and it does set it to true when a reported bug should suppress other bugs along the path.


http://reviews.llvm.org/D12445