[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

It is agreed that it is untenable to support reentrancy for `std::function` 
assignment operators and I'm not trying to achieve that. Here we restrict 
reentrancy only to `std::function::operator=(nullptr_t)`.

@mclow.lists where should the tests go if the standard specifies the 
functionality as implementation-defined?

> Except where explicitly specified in this standard, it is 
> implementation-defined which functions in the Standard C++ library may be 
> recursively reentered.

[reentrancy] p17.6.5.8


Repository:
  rCXX libc++

https://reviews.llvm.org/D34331



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

[Billy Robert O'Neal III via Phabricator via cfe-commits]

BillyONeal added subscribers: STL_MSFT, BillyONeal.
BillyONeal added a comment.

@mclow.lists
@STL_MSFT

Why did tests for this this go into std? [reentrancy]/1 says this isn't 
required to work. Moreover, assignments in the dtor like this *can't* work in 
the general case because they would try to overwrite the SSO space. e.g. what 
do you expect this to do?

  std::function global;
  
  struct B {
  int data = 1729;
  void operator() {}
  };
  
  struct A {
  int data = 42;
  ~A() {
  global = std::function(B{}); // whoops, constructs a B on top 
of A if Small Functor Optimization engages
  assert(data == 42);
  }
  
  void operator() {}
  };
  
  int main() {
  global = std::function(A{});
  global = nullptr;
  }




Repository:
  rCXX libc++

https://reviews.llvm.org/D34331



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

[Volodymyr Sapsai via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rCXX330885: [libcxx] func.wrap.func.con: Unset function before 
destroying anything (authored by vsapsai, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D34331?vs=143797=144037#toc

Repository:
  rCXX libc++

https://reviews.llvm.org/D34331

Files:
  include/__functional_03
  include/functional
  
test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/move_reentrant.pass.cpp
  
test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/nullptr_t_assign_reentrant.pass.cpp

Index: include/__functional_03
===
--- include/__functional_03
+++ include/__functional_03
@@ -600,19 +600,23 @@
 function<_Rp()>&
 function<_Rp()>::operator=(const function& __f)
 {
-function(__f).swap(*this);
+if (__f)
+function(__f).swap(*this);
+else
+*this = nullptr;
 return *this;
 }
 
 template
 function<_Rp()>&
 function<_Rp()>::operator=(nullptr_t)
 {
-if (__f_ == (__base*)&__buf_)
-__f_->destroy();
-else if (__f_)
-__f_->destroy_deallocate();
+__base* __t = __f_;
 __f_ = 0;
+if (__t == (__base*)&__buf_)
+__t->destroy();
+else if (__t)
+__t->destroy_deallocate();
 return *this;
 }
 
@@ -876,19 +880,23 @@
 function<_Rp(_A0)>&
 function<_Rp(_A0)>::operator=(const function& __f)
 {
-function(__f).swap(*this);
+if (__f)
+function(__f).swap(*this);
+else
+*this = nullptr;
 return *this;
 }
 
 template
 function<_Rp(_A0)>&
 function<_Rp(_A0)>::operator=(nullptr_t)
 {
-if (__f_ == (__base*)&__buf_)
-__f_->destroy();
-else if (__f_)
-__f_->destroy_deallocate();
+__base* __t = __f_;
 __f_ = 0;
+if (__t == (__base*)&__buf_)
+__t->destroy();
+else if (__t)
+__t->destroy_deallocate();
 return *this;
 }
 
@@ -1152,19 +1160,23 @@
 function<_Rp(_A0, _A1)>&
 function<_Rp(_A0, _A1)>::operator=(const function& __f)
 {
-function(__f).swap(*this);
+if (__f)
+function(__f).swap(*this);
+else
+*this = nullptr;
 return *this;
 }
 
 template
 function<_Rp(_A0, _A1)>&
 function<_Rp(_A0, _A1)>::operator=(nullptr_t)
 {
-if (__f_ == (__base*)&__buf_)
-__f_->destroy();
-else if (__f_)
-__f_->destroy_deallocate();
+__base* __t = __f_;
 __f_ = 0;
+if (__t == (__base*)&__buf_)
+__t->destroy();
+else if (__t)
+__t->destroy_deallocate();
 return *this;
 }
 
@@ -1428,19 +1440,23 @@
 function<_Rp(_A0, _A1, _A2)>&
 function<_Rp(_A0, _A1, _A2)>::operator=(const function& __f)
 {
-function(__f).swap(*this);
+if (__f)
+function(__f).swap(*this);
+else
+*this = nullptr;
 return *this;
 }
 
 template
 function<_Rp(_A0, _A1, _A2)>&
 function<_Rp(_A0, _A1, _A2)>::operator=(nullptr_t)
 {
-if (__f_ == (__base*)&__buf_)
-__f_->destroy();
-else if (__f_)
-__f_->destroy_deallocate();
+__base* __t = __f_;
 __f_ = 0;
+if (__t == (__base*)&__buf_)
+__t->destroy();
+else if (__t)
+__t->destroy_deallocate();
 return *this;
 }
 
Index: include/functional
===
--- include/functional
+++ include/functional
@@ -1818,11 +1818,7 @@
 function<_Rp(_ArgTypes...)>&
 function<_Rp(_ArgTypes...)>::operator=(function&& __f) _NOEXCEPT
 {
-if ((void *)__f_ == &__buf_)
-__f_->destroy();
-else if (__f_)
-__f_->destroy_deallocate();
-__f_ = 0;
+*this = nullptr;
 if (__f.__f_ == 0)
 __f_ = 0;
 else if ((void *)__f.__f_ == &__f.__buf_)
@@ -1842,11 +1838,12 @@
 function<_Rp(_ArgTypes...)>&
 function<_Rp(_ArgTypes...)>::operator=(nullptr_t) _NOEXCEPT
 {
-if ((void *)__f_ == &__buf_)
-__f_->destroy();
-else if (__f_)
-__f_->destroy_deallocate();
+__base* __t = __f_;
 __f_ = 0;
+if ((void *)__t == &__buf_)
+__t->destroy();
+else if (__t)
+__t->destroy_deallocate();
 return *this;
 }
 
Index: test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/move_reentrant.pass.cpp
===
--- test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/move_reentrant.pass.cpp
+++ test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/move_reentrant.pass.cpp
@@ -0,0 +1,46 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//

[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 143797.
vsapsai added a comment.

- Move tests to test/std.


https://reviews.llvm.org/D34331

Files:
  libcxx/include/__functional_03
  libcxx/include/functional
  
libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/move_reentrant.pass.cpp
  
libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/nullptr_t_assign_reentrant.pass.cpp

Index: libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/nullptr_t_assign_reentrant.pass.cpp
===
--- /dev/null
+++ libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/nullptr_t_assign_reentrant.pass.cpp
@@ -0,0 +1,46 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// 
+
+// class function
+
+// function& operator=(nullptr_t);
+
+#include 
+#include 
+
+#include "test_macros.h"
+
+struct A
+{
+  static std::function global;
+  static bool cancel;
+
+  ~A() {
+DoNotOptimize(cancel);
+if (cancel)
+  global = nullptr;
+  }
+  void operator()() {}
+};
+
+std::function A::global;
+bool A::cancel = false;
+
+int main()
+{
+  A::global = A();
+  assert(A::global.target());
+
+  // Check that we don't recurse in A::~A().
+  A::cancel = true;
+  A::global = nullptr;
+  assert(!A::global.target());
+}
Index: libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/move_reentrant.pass.cpp
===
--- /dev/null
+++ libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/move_reentrant.pass.cpp
@@ -0,0 +1,46 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// 
+
+// class function
+
+// function& operator=(function &&);
+
+#include 
+#include 
+
+#include "test_macros.h"
+
+struct A
+{
+  static std::function global;
+  static bool cancel;
+
+  ~A() {
+DoNotOptimize(cancel);
+if (cancel)
+  global = std::function(nullptr);
+  }
+  void operator()() {}
+};
+
+std::function A::global;
+bool A::cancel = false;
+
+int main()
+{
+  A::global = A();
+  assert(A::global.target());
+
+  // Check that we don't recurse in A::~A().
+  A::cancel = true;
+  A::global = std::function(nullptr);
+  assert(!A::global.target());
+}
Index: libcxx/include/functional
===
--- libcxx/include/functional
+++ libcxx/include/functional
@@ -1818,11 +1818,7 @@
 function<_Rp(_ArgTypes...)>&
 function<_Rp(_ArgTypes...)>::operator=(function&& __f) _NOEXCEPT
 {
-if ((void *)__f_ == &__buf_)
-__f_->destroy();
-else if (__f_)
-__f_->destroy_deallocate();
-__f_ = 0;
+*this = nullptr;
 if (__f.__f_ == 0)
 __f_ = 0;
 else if ((void *)__f.__f_ == &__f.__buf_)
@@ -1842,11 +1838,12 @@
 function<_Rp(_ArgTypes...)>&
 function<_Rp(_ArgTypes...)>::operator=(nullptr_t) _NOEXCEPT
 {
-if ((void *)__f_ == &__buf_)
-__f_->destroy();
-else if (__f_)
-__f_->destroy_deallocate();
+__base* __t = __f_;
 __f_ = 0;
+if ((void *)__t == &__buf_)
+__t->destroy();
+else if (__t)
+__t->destroy_deallocate();
 return *this;
 }
 
Index: libcxx/include/__functional_03
===
--- libcxx/include/__functional_03
+++ libcxx/include/__functional_03
@@ -600,19 +600,23 @@
 function<_Rp()>&
 function<_Rp()>::operator=(const function& __f)
 {
-function(__f).swap(*this);
+if (__f)
+function(__f).swap(*this);
+else
+*this = nullptr;
 return *this;
 }
 
 template
 function<_Rp()>&
 function<_Rp()>::operator=(nullptr_t)
 {
-if (__f_ == (__base*)&__buf_)
-__f_->destroy();
-else if (__f_)
-__f_->destroy_deallocate();
+__base* __t = __f_;
 __f_ = 0;
+if (__t == (__base*)&__buf_)
+__t->destroy();
+else if (__t)
+__t->destroy_deallocate();
 return *this;
 }
 
@@ -876,19 +880,23 @@
 function<_Rp(_A0)>&
 function<_Rp(_A0)>::operator=(const function& __f)
 {
-function(__f).swap(*this);
+if (__f)
+function(__f).swap(*this);
+else
+*this = 

[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists accepted this revision.
mclow.lists added a comment.

Please move the tests into test/std and commit.


https://reviews.llvm.org/D34331



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.
Herald added a subscriber: jkorous.

Ping.


https://reviews.llvm.org/D34331



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Ping.


https://reviews.llvm.org/D34331



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Are there more thoughts on this change?


https://reviews.llvm.org/D34331



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 139506.
vsapsai added a comment.

- Replace `function::operator=(nullptr);` with `*this = nullptr;`


https://reviews.llvm.org/D34331

Files:
  libcxx/include/__functional_03
  libcxx/include/functional
  
libcxx/test/libcxx/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/move_reentrant.pass.cpp
  
libcxx/test/libcxx/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/nullptr_t_assign_reentrant.pass.cpp

Index: libcxx/test/libcxx/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/nullptr_t_assign_reentrant.pass.cpp
===
--- /dev/null
+++ libcxx/test/libcxx/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/nullptr_t_assign_reentrant.pass.cpp
@@ -0,0 +1,46 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// 
+
+// class function
+
+// function& operator=(nullptr_t);
+
+#include 
+#include 
+
+#include "test_macros.h"
+
+struct A
+{
+  static std::function global;
+  static bool cancel;
+
+  ~A() {
+DoNotOptimize(cancel);
+if (cancel)
+  global = nullptr;
+  }
+  void operator()() {}
+};
+
+std::function A::global;
+bool A::cancel = false;
+
+int main()
+{
+  A::global = A();
+  assert(A::global.target());
+
+  // Check that we don't recurse in A::~A().
+  A::cancel = true;
+  A::global = nullptr;
+  assert(!A::global.target());
+}
Index: libcxx/test/libcxx/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/move_reentrant.pass.cpp
===
--- /dev/null
+++ libcxx/test/libcxx/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/move_reentrant.pass.cpp
@@ -0,0 +1,46 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// 
+
+// class function
+
+// function& operator=(function &&);
+
+#include 
+#include 
+
+#include "test_macros.h"
+
+struct A
+{
+  static std::function global;
+  static bool cancel;
+
+  ~A() {
+DoNotOptimize(cancel);
+if (cancel)
+  global = std::function(nullptr);
+  }
+  void operator()() {}
+};
+
+std::function A::global;
+bool A::cancel = false;
+
+int main()
+{
+  A::global = A();
+  assert(A::global.target());
+
+  // Check that we don't recurse in A::~A().
+  A::cancel = true;
+  A::global = std::function(nullptr);
+  assert(!A::global.target());
+}
Index: libcxx/include/functional
===
--- libcxx/include/functional
+++ libcxx/include/functional
@@ -1818,11 +1818,7 @@
 function<_Rp(_ArgTypes...)>&
 function<_Rp(_ArgTypes...)>::operator=(function&& __f) _NOEXCEPT
 {
-if ((void *)__f_ == &__buf_)
-__f_->destroy();
-else if (__f_)
-__f_->destroy_deallocate();
-__f_ = 0;
+*this = nullptr;
 if (__f.__f_ == 0)
 __f_ = 0;
 else if ((void *)__f.__f_ == &__f.__buf_)
@@ -1842,11 +1838,12 @@
 function<_Rp(_ArgTypes...)>&
 function<_Rp(_ArgTypes...)>::operator=(nullptr_t) _NOEXCEPT
 {
-if ((void *)__f_ == &__buf_)
-__f_->destroy();
-else if (__f_)
-__f_->destroy_deallocate();
+__base* __t = __f_;
 __f_ = 0;
+if ((void *)__t == &__buf_)
+__t->destroy();
+else if (__t)
+__t->destroy_deallocate();
 return *this;
 }
 
Index: libcxx/include/__functional_03
===
--- libcxx/include/__functional_03
+++ libcxx/include/__functional_03
@@ -600,19 +600,23 @@
 function<_Rp()>&
 function<_Rp()>::operator=(const function& __f)
 {
-function(__f).swap(*this);
+if (__f)
+function(__f).swap(*this);
+else
+*this = nullptr;
 return *this;
 }
 
 template
 function<_Rp()>&
 function<_Rp()>::operator=(nullptr_t)
 {
-if (__f_ == (__base*)&__buf_)
-__f_->destroy();
-else if (__f_)
-__f_->destroy_deallocate();
+__base* __t = __f_;
 __f_ = 0;
+if (__t == (__base*)&__buf_)
+__t->destroy();
+else if (__t)
+__t->destroy_deallocate();
 return *this;
 }
 
@@ -876,19 +880,23 @@
 function<_Rp(_A0)>&
 function<_Rp(_A0)>::operator=(const function& __f)
 {
-function(__f).swap(*this);
+if (__f)
+   

[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.



Comment at: libcxx/include/functional:1722-1733
 if (__f.__f_ == 0)
 __f_ = 0;
 else if ((void *)__f.__f_ == &__f.__buf_)
 {
 __f_ = __as_base(&__buf_);
 __f.__f_->__clone(__f_);
 }

Here is the similar code I've mentioned.



Comment at: libcxx/include/functional:1821
 {
-if ((void *)__f_ == &__buf_)
-__f_->destroy();
-else if (__f_)
-__f_->destroy_deallocate();
-__f_ = 0;
+function::operator=(nullptr);
 if (__f.__f_ == 0)

mclow.lists wrote:
> Couldn't this be more clearly written as `*this = nullptr;` ?
Personally I prefer to call operator explicitly but checking the code shows it 
isn't very common. Will change.



Comment at: libcxx/include/functional:1822
+function::operator=(nullptr);
 if (__f.__f_ == 0)
 __f_ = 0;

mclow.lists wrote:
> At this point `__f_ == 0` (because we just set it to 0).
> We probably don't need to do that again.
> 
This code is the same as in `function(function&& __f)`. Do you think there is 
enough value to deviate from that implementation?



Comment at: libcxx/include/functional:1843
 __f_ = 0;
+if ((void *)__t == &__buf_)
+__t->destroy();

mclow.lists wrote:
> I see this dance 4x in `` and once here. Did you give any 
> thought to making it a function of `__base`?  Maybe call it `__clear`?
> 
> Then line #1821 can be written as `__clear();`
> 
> 
Haven't really thought about that. Not sure it can be easily done. All `__base` 
classes are separate templates and don't have a common ancestor. Don't want to 
introduce macros as it doesn't look like libc++ style. You achieve the most 
consistency with current code by copy-pasting and repeating the same dance a 
few times.


https://reviews.llvm.org/D34331



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

A few small comments...




Comment at: libcxx/include/functional:1821
 {
-if ((void *)__f_ == &__buf_)
-__f_->destroy();
-else if (__f_)
-__f_->destroy_deallocate();
-__f_ = 0;
+function::operator=(nullptr);
 if (__f.__f_ == 0)

Couldn't this be more clearly written as `*this = nullptr;` ?



Comment at: libcxx/include/functional:1822
+function::operator=(nullptr);
 if (__f.__f_ == 0)
 __f_ = 0;

At this point `__f_ == 0` (because we just set it to 0).
We probably don't need to do that again.




Comment at: libcxx/include/functional:1843
 __f_ = 0;
+if ((void *)__t == &__buf_)
+__t->destroy();

I see this dance 4x in `` and once here. Did you give any 
thought to making it a function of `__base`?  Maybe call it `__clear`?

Then line #1821 can be written as `__clear();`




https://reviews.llvm.org/D34331



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 139369.
vsapsai edited the summary of this revision.
vsapsai added a comment.

- Implement the same functionality for C++98 and C++03.
- Use `DoNotOptimize` instead of `asm`.

Didn't move the tests as the standard doesn't require assignment operator to be
reentrant and we implement that not in general case but only for assigning
`nullptr`. Tests might be still worth moving, just previous arguments don't
apply anymore, as for me.


https://reviews.llvm.org/D34331

Files:
  libcxx/include/__functional_03
  libcxx/include/functional
  
libcxx/test/libcxx/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/move_reentrant.pass.cpp
  
libcxx/test/libcxx/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/nullptr_t_assign_reentrant.pass.cpp

Index: libcxx/test/libcxx/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/nullptr_t_assign_reentrant.pass.cpp
===
--- /dev/null
+++ libcxx/test/libcxx/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/nullptr_t_assign_reentrant.pass.cpp
@@ -0,0 +1,46 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// 
+
+// class function
+
+// function& operator=(nullptr_t);
+
+#include 
+#include 
+
+#include "test_macros.h"
+
+struct A
+{
+  static std::function global;
+  static bool cancel;
+
+  ~A() {
+DoNotOptimize(cancel);
+if (cancel)
+  global = nullptr;
+  }
+  void operator()() {}
+};
+
+std::function A::global;
+bool A::cancel = false;
+
+int main()
+{
+  A::global = A();
+  assert(A::global.target());
+
+  // Check that we don't recurse in A::~A().
+  A::cancel = true;
+  A::global = nullptr;
+  assert(!A::global.target());
+}
Index: libcxx/test/libcxx/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/move_reentrant.pass.cpp
===
--- /dev/null
+++ libcxx/test/libcxx/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/move_reentrant.pass.cpp
@@ -0,0 +1,46 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// 
+
+// class function
+
+// function& operator=(function &&);
+
+#include 
+#include 
+
+#include "test_macros.h"
+
+struct A
+{
+  static std::function global;
+  static bool cancel;
+
+  ~A() {
+DoNotOptimize(cancel);
+if (cancel)
+  global = std::function(nullptr);
+  }
+  void operator()() {}
+};
+
+std::function A::global;
+bool A::cancel = false;
+
+int main()
+{
+  A::global = A();
+  assert(A::global.target());
+
+  // Check that we don't recurse in A::~A().
+  A::cancel = true;
+  A::global = std::function(nullptr);
+  assert(!A::global.target());
+}
Index: libcxx/include/functional
===
--- libcxx/include/functional
+++ libcxx/include/functional
@@ -1818,11 +1818,7 @@
 function<_Rp(_ArgTypes...)>&
 function<_Rp(_ArgTypes...)>::operator=(function&& __f) _NOEXCEPT
 {
-if ((void *)__f_ == &__buf_)
-__f_->destroy();
-else if (__f_)
-__f_->destroy_deallocate();
-__f_ = 0;
+function::operator=(nullptr);
 if (__f.__f_ == 0)
 __f_ = 0;
 else if ((void *)__f.__f_ == &__f.__buf_)
@@ -1842,11 +1838,12 @@
 function<_Rp(_ArgTypes...)>&
 function<_Rp(_ArgTypes...)>::operator=(nullptr_t) _NOEXCEPT
 {
-if ((void *)__f_ == &__buf_)
-__f_->destroy();
-else if (__f_)
-__f_->destroy_deallocate();
+__base* __t = __f_;
 __f_ = 0;
+if ((void *)__t == &__buf_)
+__t->destroy();
+else if (__t)
+__t->destroy_deallocate();
 return *this;
 }
 
Index: libcxx/include/__functional_03
===
--- libcxx/include/__functional_03
+++ libcxx/include/__functional_03
@@ -600,19 +600,23 @@
 function<_Rp()>&
 function<_Rp()>::operator=(const function& __f)
 {
-function(__f).swap(*this);
+if (__f)
+function(__f).swap(*this);
+else
+function::operator=(nullptr);
 return *this;
 }
 
 template
 function<_Rp()>&
 function<_Rp()>::operator=(nullptr_t)
 {
-if (__f_ == (__base*)&__buf_)
-__f_->destroy();
-else if (__f_)
-   

[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

Ah, I got it. There's a `__functional_03` header that seems to implement 
`function` for C++03 because of manual variadic template expansions.
You have to update the operators in the header as well.


https://reviews.llvm.org/D34331



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

If you look at the AST diff between C++11 and C++03 this error still happens 
because of how `nullptr` is processed:

C++11:

  | |-CXXDestructorDecl 0x7fdab27a2498  line:24:3 used ~A 
'void (void) noexcept'
  | | `-CompoundStmt 0x7fdab27bcfb8 
  | |   |-GCCAsmStmt 0x7fdab27a2670 
  | |   `-IfStmt 0x7fdab27bcf88 
  | | |-<<>>
  | | |-ImplicitCastExpr 0x7fdab27a26e0  '_Bool' 
  | | | `-DeclRefExpr 0x7fdab27a26b8  '_Bool' lvalue Var 
0x7fdab27a23b8 'cancel' '_Bool'
  | | |-CXXOperatorCallExpr 0x7fdab27bcf40  'class 
std::__1::function' lvalue
  | | | |-ImplicitCastExpr 0x7fdab27bcf28  'class 
std::__1::function &(*)(nullptr_t) noexcept' 

  | | | | `-DeclRefExpr 0x7fdab27bcea0  'class 
std::__1::function &(nullptr_t) noexcept' lvalue CXXMethod 
0x7fdab27ab010 'operator=' 'class std::__1::function &(nullptr_t) 
noexcept'
  | | | |-DeclRefExpr 0x7fdab27a26f8  'std::function':'class std::__1::function' lvalue Var 0x7fdab27a2348 
'global' 'std::function':'class std::__1::function'
  | | | `-CXXNullPtrLiteralExpr 0x7fdab27a2720  'nullptr_t'
  | | `-<<>>

vs C++03:

  | |-CXXDestructorDecl 0x7fc72b6d80d8  line:24:3 used ~A 
'void (void)'
  | | `-CompoundStmt 0x7fc72b6dfd88 
  | |   |-GCCAsmStmt 0x7fc72b6d82a0 
  | |   `-IfStmt 0x7fc72b6dfd58 
  | | |-<<>>
  | | |-ImplicitCastExpr 0x7fc72b6d8310 

 '_Bool' 
  | | | `-DeclRefExpr 0x7fc72b6d82e8  '_Bool' lvalue Var 
0x7fc72b6d8028 'cancel' '_Bool'
  | | |-CXXOperatorCallExpr 0x7fc72b6dfd10  
'class std::__1::function' lvalue
  | | | |-ImplicitCastExpr 0x7fc72b6dfcf8 

 'class std::__1::function &(*)(struct std::__1::nullptr_t)' 

  | | | | `-DeclRefExpr 0x7fc72b6dfca0  'class 
std::__1::function &(struct std::__1::nullptr_t)' lvalue CXXMethod 
0x7fc72b6daea0 'operator=' 'class std::__1::function &(struct 
std::__1::nullptr_t)'
  | | | |-DeclRefExpr 0x7fc72b6d8328  'std::function':'class std::__1::function' lvalue Var 0x7fc72b6d7fb8 
'global' 'std::function':'class std::__1::function'
  | | | `-CXXConstructExpr 0x7fc72b6dfc68 
 
'struct std::__1::nullptr_t' 'void (const struct std::__1::nullptr_t &) 
throw()' elidable
  | | |   `-MaterializeTemporaryExpr 0x7fc72b6dfc50 
 
'const struct std::__1::nullptr_t' lvalue
  | | | `-ImplicitCastExpr 0x7fc72b6dfc38 
 
'const struct std::__1::nullptr_t' 
  | | |   `-CallExpr 0x7fc72b6d83d0 
 
'struct std::__1::nullptr_t'
  | | | `-ImplicitCastExpr 0x7fc72b6d83b8 
 
'struct std::__1::nullptr_t (*)(void)' 
  | | |   `-DeclRefExpr 0x7fc72b6d8380 
 
'struct std::__1::nullptr_t (void)' lvalue Function 0x7fc72b08d340 
'__get_nullptr_t' 'struct std::__1::nullptr_t (void)'
  | | `-<<>>


https://reviews.llvm.org/D34331



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

The tests actually do compile in C++03, but they still fail because of infinite 
recursion:

  frame #196562: 0x0001155c 
nullptr_t_assign_reentrant.pass.cpp.exe`A::~A() + 92
  frame #196563: 0x00011405 
nullptr_t_assign_reentrant.pass.cpp.exe`std::__1::__compressed_pair_elem::~__compressed_pair_elem() + 21
  frame #196564: 0x00012685 
nullptr_t_assign_reentrant.pass.cpp.exe`std::__1::__compressed_pair::~__compressed_pair() + 21
  frame #196565: 0x00012665 
nullptr_t_assign_reentrant.pass.cpp.exe`std::__1::__compressed_pair::~__compressed_pair() + 21
  frame #196566: 0x00012389 
nullptr_t_assign_reentrant.pass.cpp.exe`std::__1::__function::__func::destroy() + 25
  frame #196567: 0x000114b9 
nullptr_t_assign_reentrant.pass.cpp.exe`std::__1::function::operator=(std::__1::nullptr_t) + 57
  frame #196568: 0x0001155c 
nullptr_t_assign_reentrant.pass.cpp.exe`A::~A() + 92

Looks like prior to C++11 some different destruction behaviour is triggered 
which isn't fixed by this patch. So the tests should be either guarded by 
`UNSUPPORTED/XFAIL` or the patch should support C++03.


https://reviews.llvm.org/D34331



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In https://reviews.llvm.org/D34331#831686, @arphaman wrote:

> Don't you need `// UNSUPPORTED: c++98, c++03` since `std::function` is 
> supported in C++11 only?


The tests in libcxx/test/std/utilities/function.objects/ don't have UNSUPPORTED 
lines, and I don't see a lit.local.cfg that sets config.unsupported.  What 
would be different here?


https://reviews.llvm.org/D34331



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

Don't you need `// UNSUPPORTED: c++98, c++03` since `std::function` is 
supported in C++11 only?


https://reviews.llvm.org/D34331



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In https://reviews.llvm.org/D34331#803452, @EricWF wrote:

> In https://reviews.llvm.org/D34331#803105, @dexonsmith wrote:
>
> > In https://reviews.llvm.org/D34331#802747, @EricWF wrote:
> >
> > > @dexonsmith Mind if I hijack this and check in your changes to 
> > > `` with my tests?
> >
> >
> > Not at all.
> >
> > In https://reviews.llvm.org/D34331#802821, @EricWF wrote:
> >
> > > @dexonsmith I'm not sure it's sane to allow reentrant behavior. Could you 
> > > explain why you think it is?
> >
> >
> > It didn't seem sane to me at first either, despite this being supported by 
> > `std::unique_ptr` and `std::shared_ptr`.  I found our user fairly 
> > convincing, though:
> >
> > - They had an underlying reference counted object, so only the destruction 
> > of the last copy of A nulled out the pointer (mimicked that with the `bool 
> > cancel`).
> > - They had a callback function intended to be called once, and dropping 
> > that function on cancellation (mimicked that with a global variable). The 
> > cancel operation nulled out a `std::function`, but destroying the objects 
> > captured in the lambda in that std::function also separately decided to 
> > perform a cancel, which should have been OK. The cancel function just 
> > contained `callback = nullptr`.
>
>
> According to [reentrancy] it is implementation defined what STL functions are 
> allowed to be recursively reentered. I'm not opposed to providing reentrancy 
> where it's useful, but we would have to provide it well.
>  This is where I was running into problems while I was writing tests. There 
> are so many different ways you can reenter std::function's special members 
> that it makes it impossible for an implementation
>  to truly support reentrancy as the standard would require.
>
> If you consider that every possible copy construction or destruction of a 
> user type is a potential reentrancy point, the complexity of having 
> well-defined reentrant behavior starts to become clear.
>  Any time a copy constructor or destructor is invoked you have a potential 
> reentrancy point which, in order to provide well defined behavior, must also 
> act as a sort of _sequence point_ where the side effects of the current call 
> are visible to the reentrant callers (For example your patches use of 
> `operator=(nullptr_t)`).
>
> The methods fixed in this patch are seemingly improvements; It's clear that 
> `operator=(nullptr)` can safely be made reentrant. On the other hand consider 
> `operator=(function const& rhs)`, which is specified as equivalent to 
> `function(rhs).swap(*this)`.
>  I posit `swap` is not the kind of thing that can easily be made reentrant, 
> but that making `std::function` assignment reentrant in general clearly 
> requires it.
>
> If fixing this bug is sufficiently important I'm willing to accept that, but 
> I don't think it improves the status quo; We may have fixed a specific users 
> bug, but we haven't made these functions safely reentrant (in fact most of 
> the special members are likely still full of reentrancy bugs).


IMO, `function::operator=(nullptr_t)` is a clear, restricted subset of 
reentrancy: it's the cutely-named `std::function` equivalent of 
`unique_ptr::reset()`.  I think it's worth supporting reentrancy here.

After my own experimentation creating a testcase (and iterating with our 
internal users on motivation), I agree that supporting reentrancy for anything 
else would be untenable.


https://reviews.llvm.org/D34331



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added a comment.

In https://reviews.llvm.org/D34331#803105, @dexonsmith wrote:

> In https://reviews.llvm.org/D34331#802747, @EricWF wrote:
>
> > @dexonsmith Mind if I hijack this and check in your changes to 
> > `` with my tests?
>
>
> Not at all.
>
> In https://reviews.llvm.org/D34331#802821, @EricWF wrote:
>
> > @dexonsmith I'm not sure it's sane to allow reentrant behavior. Could you 
> > explain why you think it is?
>
>
> It didn't seem sane to me at first either, despite this being supported by 
> `std::unique_ptr` and `std::shared_ptr`.  I found our user fairly convincing, 
> though:
>
> - They had an underlying reference counted object, so only the destruction of 
> the last copy of A nulled out the pointer (mimicked that with the `bool 
> cancel`).
> - They had a callback function intended to be called once, and dropping that 
> function on cancellation (mimicked that with a global variable). The cancel 
> operation nulled out a `std::function`, but destroying the objects captured 
> in the lambda in that std::function also separately decided to perform a 
> cancel, which should have been OK. The cancel function just contained 
> `callback = nullptr`.


According to [reentrancy] it is implementation defined what STL functions are 
allowed to be recursively reentered. I'm not opposed to providing reentrancy 
where it's useful, but we would have to provide it well.
This is where I was running into problems while I was writing tests. There are 
so many different ways you can reenter std::function's special members that it 
makes it impossible for an implementation
to truly support reentrancy as the standard would require.

If you consider that every possible copy construction or destruction of a user 
type is a potential reentrancy point, the complexity of having well-defined 
reentrant behavior starts to become clear.
Any time a copy constructor or destructor is invoked you have a potential 
reentrancy point which, in order to provide well defined behavior, must also 
act as a sort of _sequence point_ where the side effects of the current call 
are visible to the reentrant callers (For example your patches use of 
`operator=(nullptr_t)`).

The methods fixed in this patch are seemingly improvements; It's clear that 
`operator=(nullptr)` can safely be made reentrant. On the other hand consider 
`operator=(function const& rhs)`, which is specified as equivalent to 
`function(rhs).swap(*this)`.
I posit `swap` is not the kind of thing that can easily be made reentrant, but 
that making `std::function` assignment reentrant in general clearly requires it.

If fixing this bug is sufficiently important I'm willing to accept that, but I 
don't think it improves the status quo; We may have fixed a specific users bug, 
but we haven't made these functions safely reentrant (in fact most of the 
special members are likely still full of reentrancy bugs).


https://reviews.llvm.org/D34331



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In https://reviews.llvm.org/D34331#802747, @EricWF wrote:

> @dexonsmith Mind if I hijack this and check in your changes to `` 
> with my tests?


Not at all.

In https://reviews.llvm.org/D34331#802821, @EricWF wrote:

> @dexonsmith I'm not sure it's sane to allow reentrant behavior. Could you 
> explain why you think it is?


It didn't seem sane to me at first either, despite this being supported by 
`std::unique_ptr` and `std::shared_ptr`.  I found our user fairly convincing, 
though:

- They had an underlying reference counted object, so only the destruction of 
the last copy of A nulled out the pointer (mimicked that with the `bool 
cancel`).
- They had a callback function intended to be called once, and dropping that 
function on cancellation (mimicked that with a global variable). The cancel 
operation nulled out a `std::function`, but destroying the objects captured in 
the lambda in that std::function also separately decided to perform a cancel, 
which should have been OK. The cancel function just contained `callback = 
nullptr`.

In https://reviews.llvm.org/D34331#802821, @EricWF wrote:

> Should the copy assignment operator allow reentrancy as well?


I'm not sure; what do you think?




Comment at: 
libcxx/test/libcxx/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/move_reentrant.pass.cpp:1
+//===--===//
+//

EricWF wrote:
> It seems like this test is testing behavior that should be required by the 
> standard, right?
> 
> If so it should live under `test/std`.
Yes, that likely is a better place.



Comment at: 
libcxx/test/libcxx/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/move_reentrant.pass.cpp:25
+  ~A() {
+asm("");
+if (cancel)

EricWF wrote:
> Is `asm("")` just to prevent optimizations? If so please use `DoNotOptimize` 
> from `test_macros.h`.
Sounds fine to me.


https://reviews.llvm.org/D34331



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added a comment.

> However there is another bug here. operator=(function&&) doesn't correctly 
> call the destructor of the functor. I'll fix that as a separate commit.

Woops, I misread the diff. There is no existing bug W.R.T. missing destructor 
calls.


https://reviews.llvm.org/D34331



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

[Eric Fiselier via Phabricator via cfe-commits]

EricWF requested changes to this revision.
EricWF added a comment.
This revision now requires changes to proceed.

@dexonsmith I'm not sure it's sane to allow reentrant behavior. Could you 
explain why you think it is? Should the copy assignment operator allow 
reentrancy as well?

However there is another bug here. `operator=(function&&)` doesn't correctly 
call the destructor of the functor. I'll fix that as a separate commit.


https://reviews.llvm.org/D34331



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added a comment.

@dexonsmith Mind if I hijack this and check in your changes to `` 
with my tests?




Comment at: 
libcxx/test/libcxx/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/move_reentrant.pass.cpp:1
+//===--===//
+//

It seems like this test is testing behavior that should be required by the 
standard, right?

If so it should live under `test/std`.



Comment at: 
libcxx/test/libcxx/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/move_reentrant.pass.cpp:25
+  ~A() {
+asm("");
+if (cancel)

Is `asm("")` just to prevent optimizations? If so please use `DoNotOptimize` 
from `test_macros.h`.


https://reviews.llvm.org/D34331



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

Ping!


https://reviews.llvm.org/D34331



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith created this revision.

Be defensive against a reentrant std::function::operator=(), in case the held 
function object has a non-trivial destructor.  Destroying the function object 
in-place can lead to the destructor being called twice.

rdar://problem/32836603


https://reviews.llvm.org/D34331

Files:
  libcxx/include/functional
  
libcxx/test/libcxx/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/move_reentrant.pass.cpp
  
libcxx/test/libcxx/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/nullptr_t_assign_reentrant.pass.cpp

Index: libcxx/test/libcxx/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/nullptr_t_assign_reentrant.pass.cpp
===
--- /dev/null
+++ libcxx/test/libcxx/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/nullptr_t_assign_reentrant.pass.cpp
@@ -0,0 +1,44 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// 
+
+// class function
+
+// function& operator=(nullptr_t);
+
+#include 
+#include 
+
+struct A
+{
+  static std::function global;
+  static bool cancel;
+
+  ~A() {
+asm("");
+if (cancel)
+  global = nullptr;
+  }
+  void operator()() {}
+};
+
+std::function A::global;
+bool A::cancel = false;
+
+int main()
+{
+  A::global = A();
+  assert(A::global.target());
+
+  // Check that we don't recurse in A::~A().
+  A::cancel = true;
+  A::global = nullptr;
+  assert(!A::global.target());
+}
Index: libcxx/test/libcxx/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/move_reentrant.pass.cpp
===
--- /dev/null
+++ libcxx/test/libcxx/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/move_reentrant.pass.cpp
@@ -0,0 +1,44 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// 
+
+// class function
+
+// function& operator=(function &&);
+
+#include 
+#include 
+
+struct A
+{
+  static std::function global;
+  static bool cancel;
+
+  ~A() {
+asm("");
+if (cancel)
+  global = std::function(nullptr);
+  }
+  void operator()() {}
+};
+
+std::function A::global;
+bool A::cancel = false;
+
+int main()
+{
+  A::global = A();
+  assert(A::global.target());
+
+  // Check that we don't recurse in A::~A().
+  A::cancel = true;
+  A::global = std::function(nullptr);
+  assert(!A::global.target());
+}
Index: libcxx/include/functional
===
--- libcxx/include/functional
+++ libcxx/include/functional
@@ -1821,11 +1821,7 @@
 function<_Rp(_ArgTypes...)>&
 function<_Rp(_ArgTypes...)>::operator=(function&& __f) _NOEXCEPT
 {
-if ((void *)__f_ == &__buf_)
-__f_->destroy();
-else if (__f_)
-__f_->destroy_deallocate();
-__f_ = 0;
+function::operator=(nullptr);
 if (__f.__f_ == 0)
 __f_ = 0;
 else if ((void *)__f.__f_ == &__f.__buf_)
@@ -1845,11 +1841,12 @@
 function<_Rp(_ArgTypes...)>&
 function<_Rp(_ArgTypes...)>::operator=(nullptr_t) _NOEXCEPT
 {
-if ((void *)__f_ == &__buf_)
-__f_->destroy();
-else if (__f_)
-__f_->destroy_deallocate();
+__base* __t = __f_;
 __f_ = 0;
+if ((void *)__t == &__buf_)
+__t->destroy();
+else if (__t)
+__t->destroy_deallocate();
 return *this;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits