2014/1/10 Richard Smith <[email protected]>:
> 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 <[email protected]> wrote:
>>
>> Ping
>>
>> 2013/12/25 David Wiberg <[email protected]>:
>> > 2013/12/25 David Wiberg <[email protected]>:
>> >> 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
>> >
>> > The attached patch checks that move constructor is used in C++11 mode
>> > as per Arthur's suggestion.
>>
>> _______________________________________________
>> cfe-commits mailing list
>> [email protected]
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
diff --git a/lib/CodeGen/CGStmt.cpp b/lib/CodeGen/CGStmt.cpp
index b18ff09..83b5fa9 100644
--- a/lib/CodeGen/CGStmt.cpp
+++ b/lib/CodeGen/CGStmt.cpp
@@ -871,7 +871,8 @@ void CodeGenFunction::EmitReturnStmt(const ReturnStmt &S) {
// FIXME: Clean this up by using an LValue for ReturnTemp,
// EmitStoreThroughLValue, and EmitAnyExpr.
- if (S.getNRVOCandidate() && S.getNRVOCandidate()->isNRVOVariable()) {
+ if (getLangOpts().ElideConstructors &&
+ S.getNRVOCandidate() && S.getNRVOCandidate()->isNRVOVariable()) {
// Apply the named return value optimization for this return statement,
// which means doing nothing: the appropriate result has already been
// constructed into the NRVO variable.
diff --git a/test/CodeGenCXX/no-elide-constructors.cpp b/test/CodeGenCXX/no-elide-constructors.cpp
new file mode 100644
index 0000000..2f860f2
--- /dev/null
+++ b/test/CodeGenCXX/no-elide-constructors.cpp
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -triple i386-unknown-unknown -fno-elide-constructors -emit-llvm -O1 -o - %s | FileCheck %s
+
+// Reduced from PR12208
+class X {
+public:
+ X();
+ X(const X&);
+ ~X();
+};
+
+// CHECK-LABEL: define void @_Z4Testv
+X Test()
+{
+ X x;
+ // Check that the copy constructor for X is called with result variable as
+ // sret argument.
+ // CHECK: call void @_ZN1XC1ERKS_(%class.X* %agg.result, %class.X* %x)
+ // Make sure that the destructor for X is called.
+ // CHECK: call void @_ZN1XD1Ev(%class.X* %x)
+ return x;
+}
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
