[cfe-commits] [Review] Partial fix for PR 11645: put destructor calls before a return statement.

[Erik Verbruggen]

I agree that leaving it as it is would make code generation easier for 
most cases. However, I think we have the return-value-slot problem anyway:

class A {
public:
   A();
   ~A();
};

A test() {
   A a;
   return a;
}

Generates:

  [B3 (ENTRY)]
    Succs (1): B2

  [B1]
    1: [B2.2].~A() (Implicit destructor)
    Succs (1): B0

  [B2]
    1:  (CXXConstructExpr, class A)
    2: A a;
    3: a
    4: [B2.3] (ImplicitCastExpr, NoOp, const class A)
    5: [B2.4] (CXXConstructExpr, class A)
    6: return [B2.5];
    7: [B2.2].~A() (Implicit destructor)
    Preds (1): B3
    Succs (1): B0

  [B0 (EXIT)]
    Preds (2): B1 B2

So to find out that we need to construct A in the return value slot in 
B2.5, we need to look ahead to B2.6.

Also, I think that Ted pointed to another test-case which is then 
incorrect:

class A {
public:
   A();
   ~A();
};

A test() {
   return A();
}

Generates:

  [B2 (ENTRY)]
    Succs (1): B1

  [B1]
    1: A() (CXXConstructExpr, class A)
    2: [B1.1] (BindTemporary)
    3: [B1.2] (ImplicitCastExpr, NoOp, const class A)
    4: [B1.3]
    5: [B1.4] (CXXConstructExpr, class A)
    6: ~A() (Temporary object destructor)
    7: return [B1.5];
    Preds (1): B2
    Succs (1): B0

  [B0 (EXIT)]
    Preds (1): B1

In the mean time I'll see if I can find out where the extra joint points 
get generated.

-- Erik.


On 17-1-12 7:18, Xu Zhongxing wrote:
> I think the first approach makes more sense from a pragmatic point of
> view. The semantics of the return stmt 'return A();' has 3 parts:
> 1. construct temp object A().
> 2. copy the object into the return value slot.
> 3. destruct A().
> Since in the analyzer the CFG is linearized, we have to no way to know
> that the temp object A() should be copied into the return value slot
> (whatever this means for the analysis engine) until we see the
> ReturnStmt. But if the dtor is put before the ReturnStmt, the temp
> object is already destructed when we want to return it.
>
> So the ReturnStmt is the indicator that the returned value should be
> prepared. We should not destruct the object before that.
>
> Plus, the ReturnStmt is not handled as block terminator in the CFG
> builder currently. It is only treated as a normal block element.
>
> On Tue, Jan 17, 2012 at 1:29 PM, Ted Kremenek <kremenek at apple.com
> <mailto:kremenek at apple.com>> wrote:
>
>     Hi Erik,
>
>     Thanks for working on this.  I've commented in the PR.
>
>     My concern is that I am having second thoughts about whether or not
>     this is the right direction.  If there are temporaries in the return
>     expression, should those destructors appear before the 'return'
>     statement (but after the subexpression of 'return' gets evaluated)?
>       I can see it both ways.
>
>     For example:
>
>        return A();
>
>     where 'A' converts to whatever the return value type should be, and
>     ~A() is non-trivial.  Technically, ~A() evaluates after the
>     enclosing statement.  We're giving 'return' special treatment in
>     your patch.
>
>     One clean representation of this is to put all the destructors after
>     the 'return' in the CFGBlock.  The other way is to have the
>     destructors appear after the subexpression of 'return', but before
>     the 'return' itself.  The former requires clients of the CFG to
>     rethink where they expect 'return' to be in a CFGBlock.
>
>     What do you think?
>
>     Ted
>
>     On Thursday, January 12, 2012 at 6:30 AM, Erik Verbruggen wrote:
>
>>     Hello,
>>
>>     Attached is a patch for one of the things mentioned in PR 11645,
>>     which
>>     is that in the CFG the destructor calls end up after the return
>>     statement.
>>
>>     -- Erik.
>>
>>     Attachments:
>>     - 0001-CFG-Partial-fix-for-PR-11645-put-destructor-calls-be.patch
>
>