[PATCH] Proposal on how to fix temporary dtors.

[Manuel Klimek]

Ping


On Wed, Jun 25, 2014 at 2:25 PM, Manuel Klimek <klimek at google.com> wrote:

> 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/20140704/2087cf6d/attachment.html>