<div dir="ltr">Please extend the test to check that the destructor is called when we turn off elision. Otherwise, this LGTM, thanks! Do you have commit access?</div><div class="gmail_extra"><br><br><div class="gmail_quote">
On Mon, Jan 6, 2014 at 8:29 AM, David Wiberg <span dir="ltr"><<a href="mailto:dwiberg@gmail.com" target="_blank">dwiberg@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Ping<br>
<div class="HOEnZb"><div class="h5"><br>
2013/12/25 David Wiberg <<a href="mailto:dwiberg@gmail.com">dwiberg@gmail.com</a>>:<br>
> 2013/12/25 David Wiberg <<a href="mailto:dwiberg@gmail.com">dwiberg@gmail.com</a>>:<br>
>> 2013/12/25 Arthur O'Dwyer <<a href="mailto:arthur.j.odwyer@gmail.com">arthur.j.odwyer@gmail.com</a>>:<br>
>>> On Wed, Dec 25, 2013 at 12:30 AM, David Wiberg <<a href="mailto:dwiberg@gmail.com">dwiberg@gmail.com</a>> wrote:<br>
>>>><br>
>>>> The attached patch adds a test-case for which incorrect code is<br>
>>>> generated and a change which makes the test pass.<br>
>>>><br>
>>>> From what I could tell the following happened:<br>
>>>> 1. Sema::computeNRVO marked a variable as suitable for NRVO<br>
>>>> 2. CodeGenFunction::EmitAutoVarAlloca handled allocating variables to<br>
>>>> the return slot based on getLangOpts().ElideConstructors and the flag<br>
>>>> set by Sema.<br>
>>>> 3. CodeGenFunction::EmitReturnStmt only looked at the flag set by Sema<br>
>>>> to determine if necessary to emit a return statement.<br>
>>>><br>
>>>> The patch changes EmitReturnStmt to require that the ElideConstructors<br>
>>>> flag is set to perform NRVO. Another option (which perhaps is better)<br>
>>>> would be to let Sema::computeNRVO return early if ElideConstructors<br>
>>>> wasn't set and remove the ElideConstructors check within<br>
>>>> EmitAutoVarAlloca. Since I haven't worked with this code before I<br>
>>>> opted for the least intrusive change.<br>
>>><br>
>>> Speaking as the last commenter on PR12208, I heartily approve anything<br>
>>> that has the potential to fix -fno-elide-constructors. :)<br>
>>> However, I think you should add at least one test of the same thing in<br>
>>> C++11 mode (where we expect the move-constructor to be called, not the<br>
>>> copy-constructor).<br>
>>><br>
>> This sounds like a good idea, I will have a look at what changes are required.<br>
>>> Would it make sense to add the exact std::string and std::map test<br>
>>> cases from the bug report?<br>
>>> <a href="http://llvm.org/bugs/show_bug.cgi?id=12208" target="_blank">http://llvm.org/bugs/show_bug.cgi?id=12208</a><br>
>> The reason I left those out was to avoid the dependencies and keep the<br>
>> test as simple as possible. I can add those if people think it's a<br>
>> good idea.<br>
>>><br>
>>> my $.02,<br>
>>> –Arthur<br>
>><br>
>> Thank you for the comments!<br>
>><br>
>> Best regards<br>
>> David<br>
><br>
> The attached patch checks that move constructor is used in C++11 mode<br>
> as per Arthur's suggestion.<br>
<br>
</div></div><div class="HOEnZb"><div class="h5">_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></div><br></div>