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

[David Wiberg]

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