On Tue, 2 Aug 2022 at 09:37, Sébastien Bini <sebastien.bini@gmail.com> wrote:
On Fri, Jun 10, 2022 at 2:04 AM Edward Catmur <ecatmur@googlemail.com> wrote:
On Wed, 8 Jun 2022 at 09:34, Sébastien Bini <sebastien.bini@gmail.com> wrote:
Hi all,

Well, yes; certainly arguing about the exact spelling is bikeshedding. One aspect I think needs further exploration is a similar facility to allow running code before the relocation of subobjects begins, and I think constructor delegation could be the facility to solve that; the other aspect is symmetry with operator=(T).

The delegating constructor will not have the ability to do memberwise relocation and to avoid the destructor call. As such using the delegating constructor to inject code right before relocation will lose the benefit of having a relocation ctor in the first place.

Yes, this is a bit unfortunate. Possibly the delegated constructor could use std::relocate on subobjects and some trick found to prevent the destructor call. Or we could return to function-try-with-init.

Why would you need to have that option? We don't support that for any other ctor (custom code before member initialisation).

We do, via constructor delegation. The need here to invoke custom code is to safely relocate objects that both directly own a resource (i.e. their destructor releases the resource) and have data members with throwing relocate. The idea is that before starting to relocate members you can create a guard object that will release the resource if a data member relocator throws, to avoid a resource leak.

What about a separate proposal that would allow you to inject custom code on any constructor, before the initializer list? I would have personally found it useful on some occasions, and could prove its full usefulness with relocation:

class T
{
public:
  T(T&& rhs) : std::unique_ptr<U> guard{MakeGuard(rhs)}, T{std::move(rhs), 0} { guard.release(); }
  // or with relocation ctor:
  T(T rhs) : auto guard{MakeGuard(rhs)}, auto guard2{MakeSecondGuard(rhs)} /* default memberwise relocation of subojects is done here automatically */ { guard2.release(); guard.release(); }
private:
  T(T&& rhs, int); // guarded ctor
};

The idea is to declare and initialize new names before the nominal list of initializers (delegating constructors or subobjects). The introduced objects would be recognized as their type and names appear clearly. Their destructor would be called at scope exit (even if due to an exception), in reverse declaration order as usual.

Yes, this is what I'm suggesting as try-with-init. The issue is finding a way to add the initialization into the; the position between the `try` and `{` of a function-try-block is currently free real estate. Adding it directly before the base-and-member-initialization list would be ambiguous both to the compiler and to the reader.

As for operator=(T), what do you have in mind? I am satisfied with leaving it implementation defined, users can freely use destruct + relocate or use swap at their own convenience.

Ah, hmm. The difficulty is that if we make it support `= default;` and that does the Obvious Thing of memberwise reloc-assign then we're setting up a footgun; it would turn into a memory leak for std::unique_ptr; memberwise reloc-assign is incorrect for resource-owning classes, even if memberwise reloc-construction is correct for them. But then again, a struct containing a non_null<unique_ptr> will need to generate a reloc-assignment operator that calls the reloc-assignment operator of non_null<unique_ptr> (since that's its only assignment operator), and it would feel weird to have a default implementation that the user can't access with `= default`. 

using N = non_null<unique_ptr<int>>;
struct S { N n; } s1 = ..., s2 = ...;
s1 = reloc s2; // calls into N::operator=(N); N::operator=(N const&) and N::operator=(N&&) are both deleted.

As you pointed out we cannot be satisfied with the default implementation, as doing the memberwise reloc-assign is wrong. I can only think of two ways of making reloc-assignments (x = reloc y;): either by using (a) std::swap, or by relying on the uncanny (b) destruct + placement new on this. Doing reloc-assignment using (a) comes at the cost of three std::relocate_at (for the swap) and one destructor call (y is destructed at the end of the swap):

    x = reloc y; // if done by std::swap, would cost 3 std::relocate_at, + destructor call on y.

Doing reloc-assign using (b) is cheaper as it only costs one std::relocate_at call and one destructor call (on x):

    x = reloc y; // could be the same as std::destroy_at(&x); std::relocate_at(&y, &x);

For (b) to work though, the destructor of y must not be called, as if it were used in reloc initialization:

    auto x = reloc y; // in this context, should x have a reloc ctor, the destructor of y is not called.

I suggest the following way to declare a reloc-assignment operator, that would use (b) implementation:

class T
{
public:
  // [...]
  T& operator=(T rhs) = reloc; // same as { std::destroy_at(this); return *std::relocate_at(&rhs, this); }
};

This declares a prvalue assignment operator, and defines it with a special implementation. We do not use = default; as users may expect memberwise relocation which is not what's happening. This = reloc; also allows us to make it a special function so it can share similarities with the reloc constructor: the destructor of rhs is not called at callee site, and rhs is not actually passed by value to the operator=() despite its signature, but by reference. This allows us to make the same set of optimizations as with the relocation constructor.

The destroy + placement new approach is considered safe. Since rhs is a prvalue (or behind the scene, a reference to a prvalue), its address cannot be (by construction) this or any subobject of *this. As such std::destroy_at(this); cannot destruct rhs as a side effect.

If an exception leaks through this reloc-assignment operator then *this will likely be in a destructed state (the exception was either thrown by std::destroy_at(this), leaving the object in a somewhat destructed state, or by std::relocate_at which will have failed to reconstruct *this properly). That destructed state may have disastrous consequences if *this is an object with automatic storage whose destructor will inevitably be called at some point (probably even during the stack unwinding incurred by the exception leak), which will result in calling a destructor on a destructed object. For this reason, I suggest that declaring this reloc-assign operator yields a compilation error if the destructor and the relocation operations are not noexcept. Note that this forcefully renders the reloc-assign operator noexcept. Also, std::relocate_at needs to be valid so the type must have a copy, move or relocation ctor.

I further suggest adding an extra declaration: T& operator=(T rhs) = reloc(auto); which will silently ignore the reloc-assignment operator declaration if the above requirements are not met (useful for template code).

Note that if a class does not provide a reloc-assignment operator, instructions such as `x = reloc y;` are still possible. reloc merely transforms y into a prvalue. Overload resolution will simply pick the move-assignment operator or copy-assignment operator instead if provided.

I'm not too sure. The thing is, this isn't too difficult to write by hand; even the exception safety stuff can be done with static_assert and requires (for reloc(auto)).

But also, there's a safer way to implement relocating assignment, which is (as always) copy-and-swap:

    T& operator=(T rhs) { rhs.swap(*this); return *this; }

We should be encouraging the use of copy-and-swap idiom (well, without the copy) rather than the unsafe destroy-and-relocate. The only problem is that there isn't a way to automatically generate a memberwise swap...