<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Jun 24, 2014 at 6:08 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"><div style="word-wrap:break-word"><div><div class="h5"><br><div><div>On Jun 19, 2014, at 22:02 , Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>> wrote:</div>
<br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Jun 20, 2014 at 5:28 AM, 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"><div style="word-wrap:break-word"><div><div><br><div><div>On Jun 19, 2014, at 14:19 , Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>> wrote:</div>
<br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Jun 19, 2014 at 6:30 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">I think the algorithm makes sense. I'm not sure it's different, though, than just passing up the first (or last) CXXBindTemporaryExpr visited for a given expression, which would look like this:<br>
<br>
// For a logical expression...<br>
VisitForTemporaryDtors(E->getLHS(), false, &LastBTE);<br>
const CXXBindTemporaryExpr *InnerBTE = nullptr;<br>
VisitForTemporaryDtors(E->getRHS(), false, &InnerBTE);<br>
InsertTempDtorDecisionBlock(InnerBTE);<br>
<br>
Are there any cases that wouldn't cover?<br></blockquote><div><br></div><div>Well, the problem is what to do if we don't have a BTE yet; if we only wanted to pass one pointer, the semantics on function exit would need to be:</div>
<div>if (BTE) { we have already found a BTE, no need to insert a new block when we encounter the next }</div><div>else { we have not yet found a BTE, so we need to insert a new block when we find one }</div><div><br></div>
<div>The "unconditional" branch would only fit with the first part, so we would need to somehow conjure non-null BTE for all unconditional entries, and then afterwards know that this is a special value, because since we didn't add an extra block (as we were running an unconditional case), we don't need a decision block.</div>
<div>I'd say that's a pretty strong argument that we at least need to pass the CXXBindTemporaryExpr* and a bool IsConditional.</div><div><br></div><div>Now it's right that we don't need to remember the Succ when we hit the conditional, and instead we could just always store the Succ when we enter a recursive visitation for a conditional branch (we need to store the Succ because we can have nested conditionals), but that seems to me like it would distribute the logic through even more places, and thus undesirable.</div>
</div></div></div></blockquote><br></div></div></div><div>My observation is that only certain expressions cause conditional branching, and that for these expressions you <i>always</i> need to introduce a new block if you find any destructors, say, in the RHS of a logical expression. So</div>
<div><br></div><div>1. if you're in a non-conditional sub-expression, it doesn't matter whether there are temporaries or not; you don't need to insert a decision branch,</div><div>2. if you're in a conditional sub-expression and there are no temporaries, you don't need to insert a decision branch,</div>
<div>3. if you're in a conditional sub-expression and there are temporaries, you can use any temporary from that subexpression as the guard.</div></div></blockquote><div><br></div><div>That is exactly the algorithm.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>So it looks to me like the only information you have to pass up from traversing the sub-expressions is a BTE from that subexpression. Everything else can be handled at the point where you start processing that subexpression.</div>
</div></blockquote><div><br></div><div>We have to pass the information down whether we are in a conditional part, so we know whether we have to start a new block when we hit the temporary.</div><div><br></div><div>If you're asking why we cannot start the block at the conditional point, the reason is that we cannot add it before we do the subexpression traversal (because we don't know yet whether there will be temporaries, and we don't want to add a block if there are no temporaries), and if we want to do it after the subexpression traversal, we'd somehow need to split blocks (as there can be nested conditionals, and multiple temporaries).</div>
</div></div></div></blockquote><br></div></div></div><div>Hm. So in order to add the condition after the subexpression, we'd have to always start a new block <i>before</i> the subexpression. That actually feels a bit cleaner to me, though—start a new block, traverse the subexpression, and if the block is empty, throw it away and go back to the block we had before. Having less things in flight like that makes me a bit less nervous about maintaining this code in the future. If you disagree, though, then what you have looks about as clean as it can get.</div>
</div></blockquote><div><br></div><div>The problem is when we hit nested branches, I think:</div><div>b || (b || t())</div><div>We hit the first ||, we add an empty block. We hit the second ||, we add an empty block. We visit t() and add it to that empty block. Pop the stack, see that we need a decision block - now we hook up the decision block to the empty block. Pop the stack. Now we have to somehow wind the empty block out of the generated structure, and hook it up correctly to the previous block. I'd expect that to be less maintainable than the current solution.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>Uh, one other problem: even though we're now sharing branches for several temporaries, we still have to clean up state for every CXXBindTemporaryExpr. That should probably be moved out of processCleanupTemporaryBranch and into ProcessTemporaryDtor.</div>
</div></blockquote><div><br></div><div>I'll do that.</div><div><br></div><div>Cheers,</div><div>/Manuel</div></div></div></div>