[cfe-commits] [patch] Bug 5077

[Douglas Gregor]

Hi Dimitri,

On Oct 8, 2009, at 11:54 PM, Dimitri Tcaciuc wrote:

> The attached is the proposed fix and test case for 5077.

Thank you! Some comments below.

> I've discussed it for some time on IRC and it seems this is not a
> perfect solution, however at this point I'm a bit stumped to find an
> alternative and I figured I would get more results with mail list
> discussions. The train of thought is the following:
>
> * Member initializers can accept one or more expressions which can
> emit temporaries that need to be cleaned up.

Right.

>   * Assuming expressions are correctly generated and use
> EmitCXXExprWithTemporaries, can we assume that there can only be one
> leftover temporary per argument?

I don't think we can assume that. Any given argument could involve  
computing many temporaries. For example, our initializer could be

	std::string() + "hello" + ", " + world

which has 4 temporaries.

> * The temporary needs to persist until after member constructor call.

Specifically, until the end of the "full expression", which is the  
member initializer itself. It doesn't have to be a constructor call to  
have temporaries.

> * CXXBaseOrMemberInitializer is not an Expr and cannot be wrapped
> with EmitCXXExprWithTemporaries

Right.

> * (?) There should be no meaningful temporaries left after member
> initializer completes.

Temporaries can live after the member initializer completes if they  
were bound to a reference member (12.2p5). In this case, the  
temporaries are destroyed at the end of the constructor.

> Thus, we can do the same thing as in EmitCXXExprWithTemporaries , that
> is perform a check right after member constructor completes and any
> new live temporaries still kicking around are safe to dispose of.
>
> Started nibbling on clang only a relatively short time ago, so
> comments and guidance are much appreciated!

I think your approach is right. However, I think that the cleanup code

+          // Cleaning up whatever temporaries occurred due to
+          // argument expressions to member intializer.
+          while (LiveTemporaries.size() > OldNumLiveTemporaries) {
+            PopCXXTemporary();
+          }

is in the wrong place, since temporaries can be created for base-class  
initializers and member initializers that don't involve constructor  
calls, in addition to the case you're currently handling. I suggest  
putting this temporary-cleanup code at the end of the "for" loop  
(which you'll need to restructure a little bit due to the "continue"),  
so that we handle temporaries the same way for the various different  
kinds of ctor-initializers.

I've attached an executable test case that shows some temporaries  
being created when the members are not class types. I believe that the  
current output is:

Before construction
Default-constructed X<1>
Destroyed X<1>
Default-constructed X<2>
Default-constructed X<3>
Destroyed X<3>
Default-constructed X<4>
In constructor body
Destroyed X<4>
Before destruction
Destroyed X<2>

Although GCC 4.2 seems to be destroying the temporary bound to  
HasMembers::i4 too early (it happens before "In constructor body").

Thanks for working on this!

	- Doug

-------------- next part --------------
A non-text attachment was scrubbed...
Name: meminit.cpp
Type: application/octet-stream
Size: 575 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20091012/cc04d67d/attachment.obj>