<div dir="ltr">The newest patch seems to miss the move constructor test from the previous patch. Committed in r198991, with that and some other issues in the test fixed up. Thanks for the patch!</div><div class="gmail_extra">
<br><br><div class="gmail_quote">On Fri, Jan 10, 2014 at 1:32 PM, 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">
2014/1/10 Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>>:<br>
<div class="im">> Please extend the test to check that the destructor is called when we turn<br>
> off elision. Otherwise, this LGTM, thanks! Do you have commit access?<br>
</div>Thanks for the review, attached is a patch where the test has been<br>
updated. I do not have commit access.<br>
<div class="HOEnZb"><div class="h5"><br>
><br>
><br>
> On Mon, Jan 6, 2014 at 8:29 AM, David Wiberg <<a href="mailto:dwiberg@gmail.com">dwiberg@gmail.com</a>> wrote:<br>
>><br>
>> Ping<br>
>><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>><br>
>> >>> 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<br>
>> >>>> Sema<br>
>> >>>> to determine if necessary to emit a return statement.<br>
>> >>>><br>
>> >>>> The patch changes EmitReturnStmt to require that the<br>
>> >>>> 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<br>
>> >> 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>
>> _______________________________________________<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>
><br>
><br>
</div></div></blockquote></div><br></div>