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

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

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

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

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

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 ).

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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