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