[PATCH] D85796: [Analysis] Bug fix for exploded graph branching in evalCall for constructor

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Thanks!!

I'm having second thoughts on re-using the existing builder. Most other `runCheckers...()` methods are building their own. Given how confusing this entire node builder business is, i believe we should not deviate from known working patterns.

Please forgive me if i at some point decide to take over this patch and do this myself. I have strong opinions about this entire NodeBuilder API and i'm pretty much screaming internally when i try to understand why it's made the way it's made, so there's a high chance i'll want to introduce some invasive sanity into it on my own.



================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:619
       getCheckerManager().runCheckersForEvalCall(DstEvaluated, *I, *Call, *this,
-                                                 CallOpts);
+                                                 CallOpts, Bldr);
   }
----------------
We should probably delete the copy-constructor for node builders. I've no idea what it's supposed to do anyway and the whole problem that we're having here is due to there being //too many// of them already.


================
Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:39
 void derefAfterRelease() {
-  std::unique_ptr<A> P(new A());
+  std::unique_ptr<A> P(new A()); // expected-note {{Smart pointer 'P' is constructed}}
   P.release(); // expected-note {{Smart pointer 'P' is released and set to null}}
----------------
Ok, these notes shouldn't be there; a note on `.release()` is sufficient to understand the warning and it looks like that's one more place where we should mark the region as uninteresting.

Can you try to debug why did they suddenly show up?


================
Comment at: clang/test/Analysis/smart-ptr.cpp:143
     pass_smart_ptr_by_const_rvalue_ref(std::move(P));
-    P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+    P->foo(); // no-warning
   }
----------------
That's indeed the intended consequence of your patch: we're no longer exploring a bogus execution path that's parallel to the previous null dereference on line 133, therefore we're no longer emitting this warning but instead correctly interrupting the entire analysis after we've found that other null dereference.

That said, we should preserve the test so that it was still testing whatever it was testing previously. Can you split up this function into smaller functions so that each test was running independently?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85796/new/

https://reviews.llvm.org/D85796