inplace_function: Switch conversion operators to converting constructors.

[Quuxplusone]

And two tests, which fail before this patch and succeed after it.

Fixes #125.

@Voultapher: ping!

[Voultapher]

I'd prefer to see uniform initialization, makes it easier to grog what is a function call and what is a constructor when reading, so inplace_function{...}.

Small nitpick, when possible it be great to stick to 79 char line length limit. Really helps with readability.

[Quuxplusone]

I invariably prefer to see parens rather than curlies, except when giving an initializer-list (vector<int> v = {1,2,3};) or for some reason when naming a tag type (std::monostate{}). For certain types, parens do something different from curlies. However, since in this case parens and curlies do the same thing, I guess I don't mind using curlies.

[Quuxplusone]

Actually, I take it back: here we're not just whatever-initializing a member, we're creating a delegating constructor. I'd really prefer to keep inplace_function(...) here.

[Voultapher]

Does that meet strong exception guarantees? Before we set the empty state before moving. Now we do it afterwards, what happens if the move throws.

[Quuxplusone]

I believe #138 is related to this comment. I'll take a look and see if anything special ought to be done here.

[Quuxplusone]

Let's say for the sake of argument that calling other.vtable_ptr_->relocate_ptr(&storage_, &other.storage_) did throw an exception. That exception must have come either from the wrapped object's move-constructor or the wrapped object's destructor. Destructors don't throw, so it must have come from the move-constructor.
Therefore the move-constructor did not complete; therefore this inplace-function does not contain anything in need of cleanup (which is good because we're not going to run ~inplace_function). Also therefore the destructor did not get entered; therefore other does still contain an object of the wrapped type which is in need of destruction; therefore it is very important that other.vtable_ptr_ retain its old value at this point.

HOWEVER! After #128 / #138, we already require that the contained object's move-constructor and destructor must never throw exceptions. We don't require that they actually be noexcept, because there's lots of codebases that don't use noexcept on move-constructors like they should. But if moving the contained object throws, then we call that "library-level undefined behavior" and we call std::terminate. So really the appropriate fix here is to indicate that inplace_function<Sig, Cap>(inplace_function<Sig, Cap-1>&&) is exactly as noexcept as inplace_function<Sig, Cap>(inplace_function<Sig, Cap>&&).

[Voultapher]

I've been thinking about it a bit more, and it's not clear to me if we need to expect the contained closure move constructor to be noexcept. Should the contained move constructor throw, the empty assign never happens, and rhs remains in the state the move constructor left it in when throwing, so ultimately remaining in a valid state, boils down whether the closure move constructor meets the strong exception guarantee. In that light assigning empty to the moved from object after the move, seems like the correct order.

With all this said I noticed another potential issue here. I know that C++ expects moved from objects to be in 'some' valid state, so that running the destructor doesn't cause UB. Does C++ also expect that not only the destructor can be run, but that it must be run? If so then by assigning empty to the moved from rhs here without calling the contained destructor, that would leave the contained closure in an under destroyed state.

[Quuxplusone]

Should the contained move constructor throw,

...then inplace_function would have to propagate that exception up the stack, which means inplace_function itself could never be noexcept, which means you couldn't make a vector<inplace_function<void()>> without getting the vector pessimization. We must make inplace_function nothrow-move-constructible in order to avoid the vector pessimization, and that means that the contained functor can't throw on move-construct. Period.

With all this said I noticed another potential issue here. I know that C++ expects moved from objects to be in 'some' valid state, so that running the destructor doesn't cause UB. Does C++ also expect that not only the destructor can be run, but that it must be run? If so then by assigning empty to the moved from rhs here without calling the contained destructor, that would leave the contained closure in an under destroyed state.

I'm not sure I understand your premise here. Are you perhaps forgetting that relocate_ptr does invoke the destructor on the source object after moving-out-of it (that's what "relocate" means)? Or am I missing something?

[Voultapher]

First of all, I did miss the noexcept on the move constructor and forgot that relocate_ptr invokes the destructor. With that out of the way I don't see why this shouldn't be merged as is.

On a bigger picture this discussion goes to show that having a type erased box for all types conflicts with C++'s type system. There are properties that are specific to the type that we need to declare at compile time once, for all instances of that type, most notably:

• operator() marked const
• move constructor marked noexcept

By choosing some default for all types, in this case the most permissive and most efficient one respectively, we run into problems eventually, and while having sensible defaults is important, this truly is a case of one size does not fit all.
const operator() makes std::function and by design proxy inplace_function thread hostile as it opens a very easy hole to violate const correctness. And noexcept move constructor, could cause unexpected and unwanted program termination.

In my opinion the fundamental issue here is, that no matter which defaults we choose, as long as we allow the user to put in arbitrary and incompatible types, we'll always have surprising and sub optimal behavior.

My suggestion how to fix this, haven't thought about this for very long, so there might be some glaring issues I'm missing. Still repeating the same design mistakes that std::function did seems like a serious mistake.

Bikeshed syntax and names, could be done differently:

• inplace_function statically enforced const calling operator for contained closure, and statically enforced noexcept move
• mut_inplace_function non const calling operator, and statically enforced noexcept move
• inplace_function_with_throwing_move const calling operator for contained closure, and not noexcept move
• mut_inplace_function_with_throwing_move non const calling operator, and not noexcept move

Plus one of each variant that is move only. While it's clear that having 8 different types is harder to teach and a bigger surface to maintain, it could be worth it, given that with this less permissive model, the problem arises at compile time, when the user tries to mix incompatible types in the same box, in contrast to now, where our current defaults lead to nasty run time behavior, eg race conditions and program termination.

[Quuxplusone]

On a bigger picture this discussion goes to show that having a type-erased box for all types conflicts with C++'s type system. There are properties that are specific to the type that we need to declare at compile time once, for all instances of that type, most notably:

• operator() marked const
• move constructor marked noexcept

Yes, and many more. (The next-biggest one might be "does it require copyability.") See https://quuxplusone.github.io/blog/2019/03/27/design-space-for-std-function/ .

FWIW, I do think that the two you picked have well-established "right answers":

• const should be part of the function signature type (e.g. F<void()> should not be const-callable, and F<void() const> should be const-callable). SG14 inplace_function hasn't implemented this yet, but it probably should.

• Move-construction, move-assignment, swap, and destruction should always be nothrow. There is no realistic use-case for a throwing move operation (especially in a type-erased type, where move-of-F is implemented in terms of relocate-of-T). There is normally no need to annotate nothrow operations with noexcept — except the move-constructor, because if you don't annotate your move-constructor, you get the vector pessimization.

[Quuxplusone]

Opened #158 for inplace_function<void() const>.

(Nothrow-callability is also a knob that I have actually seen coworkers wanting to use in practice. inplace_function<void() noexcept> is the same type as inplace_function<void()> in C++14-and-earlier, but it is a distinct, ill-formed, type in C++17-and-later. I don't think the return on investment is high enough to justify implementing inplace_function<void() noexcept>, though.)

[Voultapher]

This is a mix of 1TBS and Allman, let's pick one and stick to it.

[Quuxplusone]

I'd say this style is just copied from ThrowingFunctor and so on from higher up in the file, and it seems consistent to me. If you want to clang-format the code later, I'm okay with that.