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

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.

Looks good to me. I have a comment about simplifying createTemporaryRegionIfNeeded() (if possible) inline.



================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:281
+
+    if (!TR) {
+      StorageDuration SD = MT->getStorageDuration();
----------------
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?


================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:289
   }
   if (!TR)
     TR = MRMgr.getCXXTempObjectRegion(Init, LC);
----------------
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.



================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:490
 
+static void printTemporaryMaterializatinosForContext(
+    raw_ostream &Out, ProgramStateRef State, const char *NL, const char *Sep,
----------------
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.


================
Comment at: test/Analysis/lifetime-extension.cpp:45
+  clang_analyzer_eval(z == 2);
+#ifdef TEMPORARIES
+ // expected-warning at -4{{TRUE}}
----------------
Yay!


================
Comment at: test/Analysis/lifetime-extension.cpp:78
+  {
+    const C &c = C(1, &after, &before);
+  }
----------------
Nice.


https://reviews.llvm.org/D43497