On Mon, Aug 22, 2022 at 12:41 PM Edward Catmur <ecatmur_at_[hidden]>
wrote:

> On Mon, 22 Aug 2022 at 09:45, Sébastien Bini <sebastien.bini_at_[hidden]>
> wrote:
>
>> But the default implementation does memberwise reloc-assignment. How is
>> each subobject passed down to each reloc-assign without involving a copy at
>> each call (and recursively down to the smallest parts)? That cannot be done
>> without the aliasing I guess.
>>
>
> A compiler-generated default implementation that is (implicitly or
> explicitly) declared as defaulted can use aliasing, and probably should be
> required to do so. If that isn't the case (say the relocating assignment
> operator is defined as defaulted out-of-line in a separate TU, and LTO
> isn't in use) then yes, without aliasing a subobject of that type would
> need to be relocated (not copied as such) into the relocating assignment
> operator parameter slot; and this could occur recursively.
>
> I would expect that this would not in practice be a problem for
> performance, but perhaps we should mandate aliasing in this case: passing a
> prvalue (i.e. not a glvalue, which is necessarily copied/moved to the
> parameter slot) to a relocating assignment operator, or recursively calling
> that relocating assignment operator from the compiler-generated relocating
> assignment operator of a containing class.
>

I'm in favor of the default operator to be aliased. However this is not
enough as if the assignment operator of each subclass is not aliased, then
you may still end-up making that many copies (or relocations).

I believe we do need to provide a way to clearly annotate the assignment
operator as aliased (that reloc keyword comes in). Also, it would serve for
assignment operators that are defaulted in the implementation file only.


> An aside; for types that have the new callee-destroy ABI (through being
>>> relocate-only, or through declaring a relocating constructor and not opting
>>> out of the new argument passing ABI), the copy part of copy-and-swap is
>>> unnecessary: swap simpliciter is sufficient, since the source object is
>>> then destroyed at the closing brace of the relocating assignment operator.
>>> But copy-and-swap is manifestly safer for use with caller-destroy types.
>>>
>>
>> I don't get that point. The copy is either elided or done by relocation
>> right (given that we have a reloc ctor)?
>>
>
> Yes, but even relocation is unnecessary; a reloc-and-swap implementation
> of T::operator=(T rhs) is written as:
>
> T::operator=(T rhs) {
> T tmp(reloc rhs); // calls T::T(T), relocating rhs into tmp and
> obviating destructor call on rhs
> tmp.swap(*this); // hand-written to swap subobjects memberwise
> } // tmp.~T(); is called at end of scope
>
> But given that T is callee-destroy (which it must be for the reloc part of
> reloc-and-swap to actually invoke the relocating constructor as opposed to
> the move constructor), this could just be:
>
> T::operator=(T rhs) {
> rhs.swap(*this); // hand-written to swap subobjects memberwise
> } // rhs.~T(); is called at end of scope (and never delayed till end of
> caller-expression)
>

Indeed, thanks for clarifying.


> I'm split about this reloc-assignment.
>>
>> - If we don't have aliasing then we have a problem with the default
>> implementation. = default does memberwise reloc-assign, and I don't know
>> how it is going to perform. Surely making recursive copies of all
>> subjobjects down to their smallest bits is not acceptable.
>> - If we have aliasing then we are fine with the default
>> implementation. But then we need to decide whether it is the responsibility
>> of the aliased-prvalue-assignment to destroy the source object.
>> - If yes, then as you pointed down, some uncautious developer may
>> easily write leaks.
>> - If not then the aliased-prvalue-assignment cannot alleviate the
>> call to the destructor of the source, even by relocating from it (we are no
>> better than move-assign)... Indeed, if the aliased-prvalue-assignment could
>> optionally destroy the source object (by relocating from it for instance),
>> then the caller-site would need to know whether it still needs to call the
>> destructor on the source. The ABI does not allow to propagate that kind of
>> information (from callee to caller). (Sure, we could change the ABI for
>> this operator only (which we are already doing with aliasing), but let's
>> not push this too far.) We don't have that kind of issue with the
>> relocation constructor as it is in charge of destroying the source object,
>> and as such the caller-site needs not to take any further action.
>> - Consequently, if we have aliasing and that
>> aliased-prvalue-assignment is not in charge of destroying the source
>> object, then we shoot ourselves in the foot, as then we cannot even
>> implement destroy-and-relocate, which would seem like a pretty obvious
>> implementation for such an operator... Likewise we would not fully support
>> relocation-only types.
>>
>> I'd very much prefer to have aliasing and the destruction of the source
>> object to be the responsibility of the operator. That would be opt-in with
>> the reloc keywork in the operator declaration, in place of virtual or
>> static. It would feel very consistent with how the reloc ctor works, and
>> the default implementation will not be deoptimized. I still don't know how
>> to make it safe to write though :/
>>
>
> The ABI would be the same as that for passing any other type by value that
> is non-trivially relocatable and has not opted out of ABI break; that is,
> it would be the responsibility of the relocating assignment operator to
> destroy its RHS operand. The only difference would be that when called as
> x = reloc y; elision of the relocating constructor on y would be mandatory
> (y would not need to be relocated into rhs of T::operator=(T rhs), since
> rhs would alias y; this is safe wrt. self-assignment since if x aliases y
> the code already has UB). The call site then knows that y has been
> destroyed and does not emit a further destructor call.
>
> However, what I think you're missing is that the destructor call would
> still be emitted automatically within the relocating assignment operator,
> unless obviated either by the assignment operator being defaulted (in which
> case there is no code for the user to get wrong) or by relocating from the
> parameter. If the user really does want to take responsibility for
> destroying the RHS operand, they would reloc into a union member (which
> hopefully would be elided).
>

Indeed, you are right. Unlike in the relocation constructor, where the
destructor of the source object is not called (unless synthesized
relocation with a delegating constructor happens), it will indeed be called
in the assignment operator (unless reloc is used on RHS).

This part should make everything work then:

   - the default implementation should work fine with aliasing,
   - the assignment operator is still in charge of destructing the source
   object, and we can write destroy-and-relocate or swap implementations.

What's still lacking though, but not a blocking issue in my opinion, is
that in the assignment operator implementation, users cannot elegantly call
a subobject's prvalue assignment operator. What if std::relocate where just
a mere cast, similar to std::move?

template <class T>
T std::relocate(const T& d) { return static_cast<T>(d); }

Then users can write: (with: `class T : B { D _d; };` )

T& T::operator=(T rhs)
{
    B::operator=(std::relocate(rhs));
    _d = std::relocate(rhs._d);
   return *this;
}

std::relocate is safe to use as it no longer leaves the source object in a
destroyed state. Temporaries are still created for B and _d subobjects
(even if their assignment operator is aliased), but it still allows for
nice, exception-safe code. The new overload resolution rules will pick the
prvalue-assignment operator if it exists, or else move or copy assignment
operator.

And we can still write std::relocate_at in terms of std::relocate:

template <class T>
T* std::relocate_at(const T* src, T* dst) { new (dst)
T{std::relocate(*src)}; } // selected only T has a relocation ctor.
// we cannot use std::construct_at as it does not have a prvalue overload,
and as such std::relocate_at is not constexpr :/

My only concern is then users will confuse reloc and std::relocate. We
mentioned a while ago to rename std::relocate due its dangerous nature, but
if it's a mere cast then that point does not hold anymore.