[PATCH] D81315: [analyzer] Warning for default constructed unique pointer dereferences

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar marked 2 inline comments as done.
vrnithinkumar added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:117-118
 
-  // TODO: Add a note to bug reports describing this decision.
-  C.addTransition(
-      State->BindExpr(Call.getOriginExpr(), C.getLocationContext(),
-                      C.getSValBuilder().makeZeroVal(Call.getResultType())));
+  if (checkDeferenceOps(Call, C))
+    return true;
+
----------------
NoQ wrote:
> By returning "true" here you're saying that operators `*` and `->` do nothing. That's not quite right; they return a value. You should not return `true` from `evalCall` unless you //fully// model the call.
> 
> I wouldn't mind moving //precondition// checks such as this one into `checkPreCall`, even if you do `evalCall` later. That would clearly separate concerns and avoid the need to figure out whether we still need to model the call after the bug-reporting routine finishes (the engine will do that for you automatically).
> 
> In fact i wouldn't mind moving all bug reporting functionality into a separate //checker//. But that's a story for another time^^
> By returning "true" here you're saying that operators * and -> do nothing. That's not quite right; they return a value. You should not return true from evalCall unless you fully model the call.

Thanks!
I completely missed this point.

> I wouldn't mind moving precondition checks such as this one into checkPreCall, even if you do evalCall later. That would clearly separate concerns and avoid the need to figure out whether we still need to model the call after the bug-reporting routine finishes (the engine will do that for you automatically).
Made changes to move dereference precondition checks into `checkPreCall`

> In fact i wouldn't mind moving all bug reporting functionality into a separate checker. But that's a story for another time^^
Cool!
I will keep this in mind.


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

https://reviews.llvm.org/D81315