3

Consider the following code:

class Test
{
  public:
    Test()
    {
    }

    Test(const Test &other) noexcept
    {
        *this = other;
    }

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

    auto operator = (const Test &other) noexcept -> Test&
    {
        // Make a copy of "other"
        // ...
        return *this;
    }

    auto operator = (Test &&other) noexcept -> Test&
    {
        // Move "other"
        // ...
        return *this;
    }
};

I would like group together move and copy constructors. From what I understood It can be achieved using std::forward like this:

template <class T>
Test(T &&other) noexcept
{
    *this = std::forward<T>(other);
}

It looks to be working well if used carefully as follows:

Test test;
Test test1 { test };
Test test2 { std::move(test) };

On the other hand it creates an infinite recursion if I try to instantiate a Test object with a value of type different than Test:

Test test { 1 }; // Creates an infinite recursion

Is there a way to restrict the instantiation of the Test object with the (r||l)values of type Test only?

Alexandre A.
  • 1,619
  • 18
  • 29
  • 3
    [OT] implementing a copy constructor in terms of assignment will fail as soon as any of the class' members is not default-constructible, not to mention it's pointless to default-construct members first and then assign a new value to each – Piotr Skotnicki Jul 19 '15 at 16:28
  • Define "creates an infinite recursion". What _actual_ behaviour do you see? – Lightness Races in Orbit Jul 19 '15 at 16:29
  • @LightnessRacesinOrbit Just a guess, but the rhs of the assignment will probably implicit-construct a temporary from the non-`Test` value or rvalue using the very constructor being implemented to do so. – WhozCraig Jul 19 '15 at 16:32
  • 1
    @WhozCraig: Yes, I guess so too, but this is a Question & Answer site, not a Guess & Answer site, so the OP shall define their problem scientifically. – Lightness Races in Orbit Jul 19 '15 at 16:35
  • 3
    Defining assignment in terms of a copy construction is sometimes useful, but the other way around is almost never a good idea. – Jonathan Wakely Jul 19 '15 at 16:48
  • @PiotrS. Good point about "not default-constructible". The purpose was to share the code between copy constructor and copy assignment. – Alexandre A. Jul 19 '15 at 17:17
  • 3
    @AlexandreA. the idiomatic way to do that is to define a correct copy constructor, and define a correct `Test::swap(Test&) noexcept` function, and then it's very easy to define the assignment operator using the [copy-and-swap idiom](http://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom) – Jonathan Wakely Jul 19 '15 at 18:36
  • @JonathanWakely Thanks, it was a good read. – Alexandre A. Jul 21 '15 at 09:31

1 Answers1

5

The infinite recursion is caused by an instance of Test being constructed in the assignment operation, which involves calling the constructor again. The reason is that you do not have assignment operators defined for anything other than a Test.

More precisely, read this:

template<typename T>
Test(T &&other)
{
    *this = forward<T>(other);
}

as this:

template<typename T>
Test(T &&other)
{
    this->operator = (Test{forward<T>(other)}); // <- Recursion here
}

for any T that is not a Test.

To resolve the problem, you would need to define an appropriately constrained assignment operator, like so:

template<typename T>
auto operator = (T &&value)
-> typename std::enable_if<
    ! std::is_same<typename std::decay<T>::type, Test>::value,
    Test &
    >::type
{
    // Assignment involving a T that is not a Test
    return *this;
}
defube
  • 2,395
  • 1
  • 22
  • 34
  • 5
    Although this solves the infinite recursion and is technically correct (so upvoted), defining copy construction in terms of assignment is still a bad design and this fix shouldn't be used, instead the design should be changed. – Jonathan Wakely Jul 19 '15 at 16:50
  • 1
    @JonathanWakely I would have suggested defining everything through constructors, though that would have required a much longer answer. This was more of a "fish" as opposed to "fishing lessons." – defube Jul 19 '15 at 17:21