[PATCH] D43497: [analyzer] Introduce correct lifetime extension behavior in simple cases.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:396
+    ProgramStateRef State, const LocationContext *FromLC,
+    const LocationContext *ToLC) {
+  const LocationContext *LC = FromLC;
----------------
a.sidorin wrote:
> EndLC? (similar to iterators)
Sounds neat, i'd make another quick patch on all things together.


================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:281
+
+    if (!TR) {
+      StorageDuration SD = MT->getStorageDuration();
----------------
dcoughlin wrote:
> Would it be safe for the body of the `if (!TR)` to be the else branch of `if constCXXTempObjectionRegion *const *TRPtr = ...` rather then its own if statement?
Yep. Fxd.


================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:289
   }
   if (!TR)
     TR = MRMgr.getCXXTempObjectRegion(Init, LC);
----------------
dcoughlin wrote:
> Would it be safe for `TR = MRMgr.getCXXTempObjectRegion(Init, LC);` to be the else branch of `if (const MaterializeTemporaryExpr *MT = dyn_cast<MaterializeTemporaryExpr>(Result))` rather than its own `if` statement?
> 
> Ideally the number paths through this function on which we call `MRMgr.getCXXTempObjectRegion()` would be small and very clear.
> 
Nope, it also covers the case when we have an MTE but it's not static. I guess it's still more clear to add it twice than to have it as a fallback for an indefinite amount of cases.


================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:490
 
+static void printTemporaryMaterializatinosForContext(
+    raw_ostream &Out, ProgramStateRef State, const char *NL, const char *Sep,
----------------
dcoughlin wrote:
> Nit: There is a typo in the name here ("Materializatinos"). I guess this is the sub-atomic particle created as a byproduct of materializing a temporary! We have a lot of materializatinos in Cupertino.
Fixed to prevent further lifetime extensino of typos through autocompletinos.


================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:503
+    Key.first->printPretty(Out, nullptr, PP);
+    if (Value)
+      Out << " : " << Value;
----------------
a.sidorin wrote:
> As I see from line 350, `Value` is always non-null. And there is same check on line 659. Am I missing something?
Yep, all of these are non-null by construction. This actually makes me wonder if it was correct to remove `VisitCXXBindTemporaryExpr` from `ExprEngine` - might have done it too early (this is what makes the similar if in `printInitializedTemporariesForContext` redundant). The `dont_forget_destructor_around_logical_op` test also suggests that - seems to actually be a regression. I guess i'm going to revert the `VisitCXXBindTemporaryExpr` change in a follow-up commit.

Also we should allow `ostream << MR` to accept null pointers. It's a global operator overload anyway.


================
Comment at: test/Analysis/lifetime-extension.cpp:45
+  clang_analyzer_eval(z == 2);
+#ifdef TEMPORARIES
+ // expected-warning at -4{{TRUE}}
----------------
dcoughlin wrote:
> Yay!
These worked before (with old lifetime extension, since temporary constructors were inlined), i only added the run-line to see that.


https://reviews.llvm.org/D43497