On Wed, Dec 25, 2013 at 12:30 AM, David Wiberg <[email protected]> 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). 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 my $.02, –Arthur _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
