11

I'm trying to put the copy-and-swap idiom into a reusable mixin:

template<typename Derived>
struct copy_and_swap
{
    Derived& operator=(Derived copy)
    {
        Derived* derived = static_cast<Derived*>(this);
        derived->swap(copy);
        return *derived;
    }
};

I intend it to be mixed in via CRTP:

struct Foo : copy_and_swap<Foo>
{
    Foo()
    {
        std::cout << "default\n";
    }

    Foo(const Foo& other)
    {
        std::cout << "copy\n";
    }

    void swap(Foo& other)
    {
        std::cout << "swap\n";
    }
};

However, a simple test shows that it is not working:

Foo x;
Foo y;
x = y;

This only prints "default" twice, neither "copy" nor "swap" is printed. What am I missing here?

fredoverflow
  • 256,549
  • 94
  • 388
  • 662
  • 1
    Maybe the compiler is providing his own version of `operator=` since you are missing one in your `Foo` class? Maybe you'll have to do `Foo::operator=(){return copy_and_swap();}` (pseudocode)? – RedX Aug 16 '11 at 14:52

6 Answers6

7

This:

 Derived& operator=(Derived copy)

doesn't declare a copy assignment operator for the base class (it has the wrong signature). So the default generated assignment operator in Foo will not use this operator.

Remember 12.8:

A user-declared copy assignment operator X::operator= is a non-static non-template member function of class X with exactly one parameter of type X, X&, const X&, volatile X& or const volatile X&.) [Note: an overloaded assignment operator must be declared to have only one parameter; see 13.5.3. ] [Note: more than one form of copy assignment operator may be declared for a class. ] [Note: if a class X only has a copy assignment operator with a parameter of type X&, an expression of type const X cannot be assigned to an object of type X.

EDIT don't do this (can you see why ?):

You can do:

template<typename Derived>
struct copy_and_swap
{
    void operator=(const copy_and_swap& copy)
    {
        Derived copy(static_cast<const Derived&>(copy));
        copy.swap(static_cast<Derived&>(*this));
    }
};

but you lose the potential copy elision optimization.

Indeed, this would assign twice the members of derived classes: once via copy_and_swap<Derived> assignment operator, and once via the derived class' generated assignment operator. To correct the situation, you'd have to do (and not forget to do):

struct Foo : copy_and_swap<Foo>
{

    Foo& operator=(const Foo& x)
    {
        static_cast<copy_and_swap<Foo>&>(*this) = x;
        return *this;
    }

private:
    // Some stateful members here
}

The moral of the story: don't write a CRTP class for the copy and swap idiom.

Alexandre C.
  • 55,948
  • 11
  • 128
  • 197
  • What if you `= delete` the compiler-generated assignment operator for Foo (assuming C++0x)? –  Aug 16 '11 at 14:58
  • `= delete` will forbid calling `operator=` on Foo, which is counterproductive to this. – R. Martinho Fernandes Aug 16 '11 at 15:02
  • @R. Martinho: no, it is that you force the base class to assign the members of the derived class, and the default assignment operator of the derived class will *also* assign the members. So in the end, you assign twice. – Alexandre C. Aug 16 '11 at 15:09
  • @Mike - if you have c++0x, wouldn't you just use the 'move constructor'? – sje397 Aug 17 '11 at 03:16
1

You cannot inherit assignment operators as a special case, if memory correctly serves. I believe that they can be explicitly using'd in if you need.

Also, be careful about over use of copy-and-swap. It produces non-ideal results where the original has resources that could be re-used to make the copy, such as containers. Safety is guaranteed but optimum performance is not.

Puppy
  • 144,682
  • 38
  • 256
  • 465
  • Adding the `using copy_and_swap::operator=;` in `struct Foo`, doesn't correct the situation. See [Alexandre C.'s answer](http://stackoverflow.com/questions/7080137/reusing-the-copy-and-swap-idiom/7080212#7080212). – André Caron Aug 16 '11 at 14:56
0

The compiler automatically generates a copy assignment operator for Foo, since there is none. If you add a

    using copy_and_swap<Foo>::operator=;

to Foo you will see an error telling you about the ambiguity on g++.

mmmmmmmm
  • 15,269
  • 2
  • 30
  • 55
0

Maybe you could rewrite it so it looks like so:

template<class Derived>
struct CopySwap
{
  Dervied &operator=(Derived const &other)
  {
    return AssignImpl(other);
  }

  Derived &operator=(Dervied &&other)
  {
    return AssignImpl(std::move(other));
  }

private:
  Derived &AssignImpl(Derived other)
  {
    auto self(static_cast<Derived*>(this));
    self->swap(other);
    return *self;
  }
};

It'll probably all get inlined and likely won't be any slower than the original code.

0

I am afraid this is one area where a macro is necessary, because of the complex rules about automatically generated copy and assignment operators.

No matter what you do, you are in either of two cases:

  • You have provided (explicitly) a declaration of the assignment operator, in which case you are expected to provide a definition too
  • You have not provided (explicitly) a declaration of the assignment operator, in which case the compiler will generate one if the base classes and non-static members have one available.

The next question, therefore, is: Is it worth it to automate such writing ?

Copy-And-Swap is only used for very specific classes. I do not think it's worth it.

Matthieu M.
  • 287,565
  • 48
  • 449
  • 722
0

This does not really answer the question (@Alexandre C. already did), but if you reverse the inheritance, you could make it work:

template<typename Base>
struct copy_and_swap : Base
{
    copy_and_swap& operator=(copy_and_swap copy)
    {
        swap(copy);
        return *this;
    }
};

struct Foo_
{
    Foo_()
    {
        std::cout << "default\n";
    }

    Foo_(const Foo_& other)
    {
        std::cout << "copy\n";
    }

    void swap(Foo_& other)
    {
        std::cout << "swap\n";
    }
};

typedef copy_and_swap<Foo_> Foo;

int main()
{
    Foo x;
    Foo y;
    x = y;
}
Community
  • 1
  • 1
Luc Touraille
  • 79,925
  • 15
  • 92
  • 137