[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 `<functional>` 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