Consolidate mutual calls of move/copy assign and move/copy c'tor

[mossmaurice]

Brief refactoring description

Throughout the code base, there different strategies are used to avoid code duplication when it comes to move/copy assignment operator and move/copy c'tor. This should be consolidated according to comment below.

Detailed information

From @MatthiasKillat this comment:

Foo::Foo(Foo&& other) 
{
    *this = std::move(other);
}

1. This way we have the self assignment assignment check here (but self-assignment cannot happen in the ctor!)
1. We default initialize and then immediately assign. The compiler cannot prevent this in all cases as a member ctor call is programmer intention that cannot be simply ignored. This is mainly an issue if there are non primitive types, more so if they are large.

Short story is that the correct way is to use the initializer list to initialize the members (if they depend on ctor args). To reduce code duplication one can either use a helper function or implement assignment in terms of copy via copy-and-swap idiom.

[FerdinandSpitzschnueffler]

What about the copy c'tor/assignment operator? The argumentation from above is also true for copy.

[elfenpiff]

@mossmaurice @MatthiasKillat

First of all, we use the same approach also in the copy constructor and I would assume that Matthias argument applies to this as well.

The argument why we pursued this approach was:

• it is future proof, when we add new members we have to adjust two places (copy & move assignment) instead of four places. We had this bug in the past and this was our lessons learned.
• It reduces code duplication. Important since sometimes we forget to invalidate the origin in the move assignment/ctor, also a bug of the past.

Sometimes we cannot pursue this approach since the members are not trivially constructible or in very rare cases, it would be too time consuming.

For now I would propose that we do not change anything in our codebase. In the near future we have to make all our constructs at least weak exception safe which would mean that we have to pursue a different approach and this is the time where we should tackle this challenge safely and efficiently .

[elfenpiff]

In my opinion this should not be part of the 3.0 release. It is part of the release where we remove noexcept.

[elBoberido]

I also think this is not required to be in v3.0. This is a non-breaking change and can be done anytime.

@elfenpiff the issue is two fold. I think we all agree on removing the check for this in the ctor, e.g. by having a copy(const T&) and call this from the ctor and the assignment operator. The latter with the this check.

The more controversial part is to do copy/move via the initializer list. I can see the point in trying to prevent the useless default initialization, especially for cases where this is not negligible.

Another freaky solution would be this

// disclaimer: don't do this at home kids

class Foo
{
  public:
    Foo() = default;
    Foo(const Foo& other)
        : data(other.data)
    {
    }
    Foo(Foo&& other)
        : data(std::move(other.data))
    {
        other.data = 0;
    }

    Foo& operator=(const Foo& rhs)
    {
        if (this != &rhs)
        {
            this->~Foo();
            new (this) Foo(rhs);
        }
        return *this;
    }
    Foo& operator=(Foo&& rhs)
    {
        if (this != &rhs)
        {
            this->~Foo();
            new (this) Foo(std::move(rhs));
        }
        return *this;
    }

    uint32_t data{0};
};

A cold shiver ran down my spine while writing that code. I never used it myself and never saw something like that but it seems to work 😬

There are issues with exception though and maybe other stuff like unintentional double deletes ... better you forget about this 😅