<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Jun 11, 2014 at 7:06 PM, Jordan Rose <span dir="ltr"><<a href="mailto:jordan_rose@apple.com" target="_blank">jordan_rose@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Let's bring it home!<br></blockquote><div><br></div><div>We still need to decide what to do with the FoldingSetNode patch - if you prefer, I can just inline it into this patch into the .cc file, which would still allow us to decide to put it into a more common place later?</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Thanks again for working on this. I'm excited to have it in trunk and hopefully soon to turn it on by default.<br>
<div class=""><br>
================<br>
Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:521-527<br>
@@ +520,9 @@<br>
+  }<br>
+  StmtNodeBuilder StmtBldr(PreVisit, Dst, *currBldrCtx);<br>
+  ProgramStateRef State = Pred->getState();<br>
+  assert(!State->contains<InitializedTemporariesSet>(<br>
+      std::make_pair(BTE, Pred->getStackFrame())));<br>
+  State = State->add<InitializedTemporariesSet>(<br>
+      std::make_pair(BTE, Pred->getStackFrame()));<br>
+  StmtBldr.generateNode(BTE, Pred, State);<br>
+}<br>
----------------<br>
</div><div class="">Manuel Klimek wrote:<br>
> Jordan Rose wrote:<br>
> > Right, this is not correct. We need to iterate over the nodes in the pre-visit set and update //all// of them to generate `Dst`.<br>
> /me headdesks. Done. I'm still not sure I understand all the necessary pieces to generate new nodes (and have no idea how to write tests that trigger the corner cases), so I'd appreciate another careful look (or some test ideas).<br>

</div>I'm not sure about the tests offhand, either—most likely it's currently untestable because we have no pre-statement checkers looking for CXXBindTemporaryExprs. The general principle, though, is that anything that has an ExplodedNodeSet out parameter has the potential to generate new nodes.<br>

<br>
================<br>
Comment at: test/Analysis/temporaries.cpp:283-288<br>
@@ +282,8 @@<br>
+<br>
+  bool testRecursiveFrames(bool isInner) {<br>
<div class="">+    if (isInner || check(NoReturnDtor()) || testRecursiveFrames(true)) {<br>
</div>+      clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}<br>
+    }<br>
+  }<br>
+  void testRecursiveFramesStart() { testRecursiveFrames(false); }<br>
+<br>
----------------<br>
I know this was my example, but I'm concerned that testRecursiveFrames will be evaluated as top-level as well, in which case the inner case is trivially reached. Let's make sure it is trying the !isInner case around with a little hack:<br>

<br>
    if (isInner ||<br>
        (clang_analyzer_warnIfReached(), false) || // expected-warning{{REACHABLE}}<br>
        check(NoReturnDtor()) ||<br>
        testRecursiveFrames(true)) {<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<a href="http://reviews.llvm.org/D3627" target="_blank">http://reviews.llvm.org/D3627</a><br>
<br>
<br>
</blockquote></div><br></div></div>