6

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;
}
  • 1
    I wouldn't dare to end the lifetime of an object and begin a new one in its place at least not when the class is not `final`. – j6t Oct 08 '19 at 05:33
  • Good point @j6t. I've now added a is_final check to the sample code. – Fredrik Orderud Oct 08 '19 at 06:56
  • 1
    This obviously breaks for self-move-assignment, which must still (not have undefined behavior and) leave the object valid. Why not do the move-and-swap thing? – Davis Herring Oct 08 '19 at 07:19
  • Good point @DavisHerring. I've now added move-self-assign detection to the code sample above. Please note that the constructors will still perform std::swap or std::exchange of each class member, if that is what you're referring to. Or am I misunderstanding? – Fredrik Orderud Oct 08 '19 at 07:27
  • @FredrikOrderud: I’m saying that you can, given `swap` (which is already a good idea), just [write](https://en.cppreference.com/w/cpp/language/operators#Assignment_operator) `T& operator=(T t) {swap(t); return *this;}`. It’s efficient just as often as the destructor-based approach. – Davis Herring Oct 08 '19 at 07:47
  • I personally prefer to destroy the existing object first, instead of swapping state with the moved-from object. That way, I avoid confusing behavior related to the existing object lifetime being extended to the caller. I guess that's a matter of taste(?) – Fredrik Orderud Oct 08 '19 at 08:32
  • 1
    Most people should use swap idiom and avoid potential undefined behavior. For example, your code would allows to modify a constant or reference member which is unexpected from compiler or user code. – Phil1970 Oct 10 '19 at 04:18
  • Good point @Phil1970 . I've now added a comment in the sample code to document the const/reference limitation. – Fredrik Orderud Oct 10 '19 at 12:44

1 Answers1

4

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.

Davis Herring
  • 24,173
  • 3
  • 25
  • 55
L. F.
  • 16,219
  • 7
  • 33
  • 67
  • Thanks for pointing out the UB property. I was not aware of it. Do you have reference to where I can learn more about "inplace destroy+reconstruct" being UB? My reason for so far not using "swap" methods have been to avoid confusing behavior related to the lifetime of the original object being extended as described on http://scottmeyers.blogspot.com/2014/06/the-drawbacks-of-implementing-move.html . However, I might need to reconsider if the alternative is UB... – Fredrik Orderud Oct 08 '19 at 12:33
  • 1
    @FredrikOrderud: Scott Meyers’ objection applies only to the case with an rvalue-reference parameter. The UB is implicit in [[basic.life]/6–8](http://eel.is/c++draft/basic.life#8). – Davis Herring Oct 08 '19 at 13:07
  • 1
    Be careful that you're not calling `std::swap` above. That would lead to infinite recursion. – Howard Hinnant Oct 08 '19 at 13:08
  • @FredrikOrderud You are welcome. I have updated my answer to include the standard reference on this topic. – L. F. Oct 08 '19 at 13:31
  • 1
    Note that [basic.life]/8 *saves* you in the absence of const/reference members. – Davis Herring Oct 08 '19 at 15:08
  • @DavisHerring Sorry for spreading nonsense. I have struck the wrong sentence out. – L. F. Oct 09 '19 at 08:06
  • Pointers are trivial types, so from a std/LL POV **on common arch (flat memory, simple ptrs represented as a number) you never need `std::launder`**. This is unlikely to be true on any real world impl, or intended. – curiousguy Oct 11 '19 at 23:12