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

[Richard Smith]

The newest patch seems to miss the move constructor test from the previous
patch. Committed in r198991, with that and some other issues in the test
fixed up. Thanks for the patch!


On Fri, Jan 10, 2014 at 1:32 PM, David Wiberg <dwiberg at gmail.com> wrote:

> 2014/1/10 Richard Smith <richard at metafoo.co.uk>:
> > 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?
> Thanks for the review, attached is a patch where the test has been
> updated. I do not 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/20140110/7fd6ffcf/attachment.html>