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

David Wiberg dwiberg at gmail.com
Mon Jan 6 08:29:21 PST 2014


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.




More information about the cfe-commits mailing list