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

[Richard Smith]

Please extend the test to check that the destructor is called when we turn
off elision. Otherwise, this LGTM, thanks! Do you have commit access?


On Mon, Jan 6, 2014 at 8:29 AM, David Wiberg <dwiberg at gmail.com> wrote:

> 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.
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140109/b785197a/attachment.html>