Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Is it legal to implement assignment operators as "destroy + construct"?

I frequently need to implement C++ wrappers for "raw" resource handles, like file handles, Win32 OS handles and similar. When doing this, I also need to implement move operators, since the default compiler-generated ones will not clear the moved-from object, yielding double-delete problems.

When implementing the move assignment operator, I prefer to call the destructor explicitly and in-place recreate the object with placement new. That way, I avoid duplication of the destructor logic. In addition, I often implement copy assignment in terms of copy+move (when relevant). This leads to the following code:

/** Canonical move-assignment operator. 
    Assumes no const or reference members. */
TYPE& operator = (TYPE && other) noexcept {
    if (&other == this)
        return *this; // self-assign

    static_assert(std::is_final<TYPE>::value, "class must be final");
    static_assert(noexcept(this->~TYPE()), "dtor must be noexcept");
    this->~TYPE();

    static_assert(noexcept(TYPE(std::move(other))), "move-ctor must be noexcept");
    new(this) TYPE(std::move(other));
    return *this;
}

/** Canonical copy-assignment operator. */
TYPE& operator = (const TYPE& other) {
    if (&other == this)
        return *this; // self-assign

    TYPE copy(other); // may throw

    static_assert(noexcept(operator = (std::move(copy))), "move-assignment must be noexcept");
    operator = (std::move(copy));
    return *this;
}

It strikes me as odd, but I have not seen any recommendations online for implementing the move+copy assignment operators in this "canonical" way. Instead, most websites tend to implement the assignment operators in a type-specific way that must be manually kept in sync with the constructors & destructor when maintaining the class.

Are there any arguments (besides performance) against implementing the move & copy assignment operators in this type-independent "canonical" way?

UPDATE 2019-10-08 based on UB comments:

I've read through http://eel.is/c++draft/basic.life#8 that seem to cover the case in question. Extract:

If, after the lifetime of an object has ended ..., a new object is created at the storage location which the original object occupied, a pointer that pointed to the original object, a reference that referred to the original object, ... will automatically refer to the new object and, ..., can be used to manipulate the new object, if ...

There's some obvious conditions thereafter related to the same type and const/reference members, but they seem to be required for any assignment operator implementation. Please correct me if I'm wrong, but this seems to me like my "canonical" sample is well behaved and not UB(?)

UPDATE 2019-10-10 based on copy-and-swap comments:

The assignment implementations can be merged into a single method that takes a value argument instead of reference. This also seem to eliminate the need for the static_assert and self-assignment checks. My new proposed implementation then becomes:

/** Canonical copy/move-assignment operator.
    Assumes no const or reference members. */
TYPE& operator = (TYPE other) noexcept {
    static_assert(!std::has_virtual_destructor<TYPE>::value, "dtor cannot be virtual");
    this->~TYPE();
    new(this) TYPE(std::move(other));
    return *this;
}
like image 831
Fredrik Orderud Avatar asked Oct 08 '19 04:10

Fredrik Orderud


1 Answers

There is a strong argument against your "canonical" implementation — it is wrong.

You end the lifetime the original object and create a new object in its place. However, pointers, references, etc. to the original object are not automatically updated to point to the new object — you have to use std::launder. (This sentence is wrong for most classes; see Davis Herring’s comment.) Then, the destructor is automatically called on the original object, triggering undefined behavior.

Reference: (emphasis mine) [class.dtor]/16

Once a destructor is invoked for an object, the object no longer exists; the behavior is undefined if the destructor is invoked for an object whose lifetime has ended.Example: If the destructor for an automatic object is explicitly invoked, and the block is subsequently left in a manner that would ordinarily invoke implicit destruction of the object, the behavior is undefined. — end example ]

[basic.life]/1

[...] The lifetime of an object o of type T ends when:

  • if T is a class type with a non-trivial destructor ([class.dtor]), the destructor call starts, or

  • the storage which the object occupies is released, or is reused by an object that is not nested within o ([intro.object]).

(Depending on whether the destructor of your class is trivial, the line of code that ends the lifetime of the object is different. If the destructor is non-trivial, explicitly calling the destructor ends the lifetime of the object; otherwise, the placement new reuses the storage of the current object, ending its lifetime. In either case, the lifetime of the object has been ended when the assignment operator returns.)


You may think that this is yet another "any sane implementation will do the right thing" kind of undefined behavior, but actually many compiler optimization involve caching values, which take advantage of this specification. Therefore, your code can break at any time when the code is compiled under a different optimization level, by a different compiler, with a different version of the same compiler, or when the compiler just had a terrible day and is in a bad mood.


The actual "canonical" way is to use the copy-and-swap idiom:

// copy constructor is implemented normally
C::C(const C& other)
    : // ...
{
    // ...
}

// move constructor = default construct + swap
C::C(C&& other) noexcept
    : C{}
{
    swap(*this, other);
}

// assignment operator = (copy +) swap
C& C::operator=(C other) noexcept // C is taken by value to handle both copy and move
{
    swap(*this, other);
    return *this;
}

Note that, here, you need to provide a custom swap function instead of using std::swap, as mentioned by Howard Hinnant:

friend void swap(C& lhs, C& rhs) noexcept
{
    // swap the members
}

If used properly, copy-and-swap incurs no overhead if the relevant functions are properly inlined (which should be pretty trivial). This idiom is very commonly used, and an average C++ programmer should have little trouble understanding it. Instead of being afraid that it will cause confusion, just take 2 minutes to learn it and then use it.

This time, we are swapping the values of the objects, and the lifetime of the object is not affected. The object is still the original object, just with a different value, not a brand new object. Think of it this way: you want to stop a kid from bullying others. Swapping values is like civilly educating them, whereas "destroying + constructing" is like killing them making them temporarily dead and giving them a brand new brain (possibly with the help of magic). The latter method can have some undesirable side effects, to say the least.

Like any other idiom, use it when appropriate — don't just use it for the sake of using it.

like image 97
L. F. Avatar answered Nov 04 '22 13:11

L. F.