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

[David Wiberg]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pr12208-2.patch
Type: application/octet-stream
Size: 1898 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131225/a1f753f0/attachment.obj>