[PATCH] D30534: [analyzer] When creating a temporary object copy, properly copy the value into it.

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin added a comment.

Hi Artem! Thank you for this patch. It looks very promising, but I have some questions and remarks.



================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:187
                                           const Expr *Result) {
-  SVal V = State->getSVal(Ex, LC);
+  SVal ExV = State->getSVal(Ex, LC);
   if (!Result) {
----------------
If we are touching names, should we rename Ex to InitWithAdjustments (or smth like this) and ExV correspondingly?


================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:281
+  // Try to recover some path sensitivity in case we couldn't compute the value.
+  if (ExV.isUnknown())
+    ExV = getSValBuilder().conjureSymbolVal(Result, LC, Ex->getType(),
----------------
Should we do all these operations with ExV/Reg if the InitV is known? There is a FIXME but I think it is related to all this code, not to the bindLoc only. And what happens if we remove this code?


================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:285
+
+  // FIXME: Why do we need to do that if WipeV was known to begin with?
+  State = State->bindLoc(Reg, ExV, LC);
----------------
Seems like WipeV in comment should be InitV?


================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:286
+  // FIXME: Why do we need to do that if WipeV was known to begin with?
+  State = State->bindLoc(Reg, ExV, LC);
+
----------------
If I understand correcly, if we call `bindLoc()`, we call `checkRegionChanges()` callbacks. And if we `bindLoc()` twice, we call them twice too. Is this what we want here?


https://reviews.llvm.org/D30534