2013/12/25 Arthur O'Dwyer <[email protected]>: > 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). > 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
