[PATCH] Proposal on how to fix temporary dtors.

[Manuel Klimek]

On Tue, Jun 24, 2014 at 6:21 PM, Manuel Klimek <klimek at google.com> wrote:

> On Tue, Jun 24, 2014 at 6:08 PM, Jordan Rose <jordan_rose at apple.com>
> wrote:
>
>>
>> On Jun 19, 2014, at 22:02 , Manuel Klimek <klimek at google.com> wrote:
>>
>> On Fri, Jun 20, 2014 at 5:28 AM, Jordan Rose <jordan_rose at apple.com>
>> wrote:
>>
>>>
>>> On Jun 19, 2014, at 14:19 , Manuel Klimek <klimek at google.com> wrote:
>>>
>>> On Thu, Jun 19, 2014 at 6:30 PM, Jordan Rose <jordan_rose at apple.com>
>>> wrote:
>>>
>>>> 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:
>>>>
>>>>     // For a logical expression...
>>>>     VisitForTemporaryDtors(E->getLHS(), false, &LastBTE);
>>>>     const CXXBindTemporaryExpr *InnerBTE = nullptr;
>>>>     VisitForTemporaryDtors(E->getRHS(), false, &InnerBTE);
>>>>     InsertTempDtorDecisionBlock(InnerBTE);
>>>>
>>>> Are there any cases that wouldn't cover?
>>>>
>>>
>>> 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:
>>> if (BTE) { we have already found a BTE, no need to insert a new block
>>> when we encounter the next }
>>> else { we have not yet found a BTE, so we need to insert a new block
>>> when we find one }
>>>
>>> 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.
>>> I'd say that's a pretty strong argument that we at least need to pass
>>> the CXXBindTemporaryExpr* and a bool IsConditional.
>>>
>>> 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.
>>>
>>>
>>> My observation is that only certain expressions cause conditional
>>> branching, and that for these expressions you *always* need to
>>> introduce a new block if you find any destructors, say, in the RHS of a
>>> logical expression. So
>>>
>>> 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,
>>> 2. if you're in a conditional sub-expression and there are no
>>> temporaries, you don't need to insert a decision branch,
>>> 3. if you're in a conditional sub-expression and there are temporaries,
>>> you can use any temporary from that subexpression as the guard.
>>>
>>
>> That is exactly the algorithm.
>>
>>
>>> 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.
>>>
>>
>> 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.
>>
>> 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).
>>
>>
>> Hm. So in order to add the condition after the subexpression, we'd have
>> to always start a new block *before* 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.
>>
>
> The problem is when we hit nested branches, I think:
> b || (b || t())
> 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.
>
>
>> 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.
>>
>
> I'll do that.
>

Done. I had to shuffle methods around a bit...


>
> Cheers,
> /Manuel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140625/e7265a3b/attachment.html>