[PATCH] PR12208 - Problem with -fno-elide-constructors

[David Wiberg]

2014/1/10 Richard Smith <richard at metafoo.co.uk>:
> 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?
Thanks for the review, attached is a patch where the test has been
updated. I do not have commit access.

>
>
> On Mon, Jan 6, 2014 at 8:29 AM, David Wiberg <dwiberg at gmail.com> wrote:
>>
>> Ping
>>
>> 2013/12/25 David Wiberg <dwiberg at gmail.com>:
>> > 2013/12/25 David Wiberg <dwiberg at gmail.com>:
>> >> 2013/12/25 Arthur O'Dwyer <arthur.j.odwyer at gmail.com>:
>> >>> On Wed, Dec 25, 2013 at 12:30 AM, David Wiberg <dwiberg at gmail.com>
>> >>> wrote:
>> >>>>
>> >>>> The attached patch adds a test-case for which incorrect code is
>> >>>> generated and a change which makes the test pass.
>> >>>>
>> >>>> From what I could tell the following happened:
>> >>>> 1. Sema::computeNRVO marked a variable as suitable for NRVO
>> >>>> 2. CodeGenFunction::EmitAutoVarAlloca handled allocating variables to
>> >>>> the return slot based on getLangOpts().ElideConstructors and the flag
>> >>>> set by Sema.
>> >>>> 3. CodeGenFunction::EmitReturnStmt only looked at the flag set by
>> >>>> Sema
>> >>>> to determine if necessary to emit a return statement.
>> >>>>
>> >>>> The patch changes EmitReturnStmt to require that the
>> >>>> ElideConstructors
>> >>>> flag is set to perform NRVO. Another option (which perhaps is better)
>> >>>> would be to let Sema::computeNRVO return early if ElideConstructors
>> >>>> wasn't set and remove the ElideConstructors check within
>> >>>> EmitAutoVarAlloca. Since I haven't worked with this code before I
>> >>>> opted for the least intrusive change.
>> >>>
>> >>> Speaking as the last commenter on PR12208, I heartily approve anything
>> >>> that has the potential to fix -fno-elide-constructors. :)
>> >>> However, I think you should add at least one test of the same thing in
>> >>> C++11 mode (where we expect the move-constructor to be called, not the
>> >>> copy-constructor).
>> >>>
>> >> This sounds like a good idea, I will have a look at what changes are
>> >> required.
>> >>> Would it make sense to add the exact std::string and std::map test
>> >>> cases from the bug report?
>> >>> http://llvm.org/bugs/show_bug.cgi?id=12208
>> >> The reason I left those out was to avoid the dependencies and keep the
>> >> test as simple as possible. I can add those if people think it's a
>> >> good idea.
>> >>>
>> >>> my $.02,
>> >>> –Arthur
>> >>
>> >> Thank you for the comments!
>> >>
>> >> Best regards
>> >> David
>> >
>> > The attached patch checks that move constructor is used in C++11 mode
>> > as per Arthur's suggestion.
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pr12208-3.patch
Type: text/x-patch
Size: 1493 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140110/c6b490c8/attachment.bin>