Safe assignment and copy-and-swap idiom [closed]

Question

I'm learning c++ and I recently learned (here in stack overflow) about the copy-and-swap idiom and I have a few questions about it. So, suppose I have the following class using a copy-and-swap idiom, just for example:

class Foo {
private:
  int * foo;
  int size;

public:
  Foo(size_t size) : size(size) { foo = new int[size](); }
  ~Foo(){delete foo;}

  Foo(Foo const& other){
    size = other.size;
    foo = new int[size];
    copy(other.foo, other.foo + size, foo);
  }

  void swap(Foo& other) { 
    std::swap(foo,  other.foo);  
    std::swap(size, other.size); 
  }

  Foo& operator=(Foo g) { 
    g.swap(*this); 
    return *this; 
  }

  int& operator[] (const int idx) {return foo[idx];}
};

My question is, suppose I have another class that have a Foo object as data but no pointers or other resources that might need custom copying or assignment:

class Bar {
private:
  Foo bar;
public:
  Bar(Foo foo) : bar(foo) {};
  ~Bar(){};
  Bar(Bar const& other) : bar(other.bar) {}; 
  Bar& operator=(Bar other) {bar = other.bar;}
};

Now I have a series of questions:

1. Are the methods and constructors as implemented above for the Bar class safe? Having used the copy-and-swap for Foo make me sure that no harm can be done when assigning or copying Bar?

2. Passing the argument by reference in the copy constructor and in swap is mandatory?

3. Is it right to say that when the argument of operator= is passed by value, the copy constructor is called for this argument to generate a temporary copy of the object, and that it is this copy which is then swapped with *this? If I passed by reference in operator= I would have a big problem, right?

4. Are there situations in which this idiom fails to provide complete safety in copying and assigning Foo?

Sentience

Your copy constructor has a rather serious bug: it does foo = new int[size] before setting size – Rahal

The custom copy constructor, assignment operator and destructor in Bar are completely unnecessary. Or did you already know that? – Flashing

Cieslak: oops! Thanks for pointing out. Lindley: Yes, I know that this is completely equivalent to the default constructors, its for the sake of explicitness, to clarify my question. – Sentience

+1: nice question, especially for a C++ beginner! – Intercostal

I strongly suggest using std::vector<int> as a member. Then you don't have to write the destructor and the copy operations in the first place. – Ache

@FredOverflow: I agree, though for the purpose of learning, it's useful to study the mechanisms :) – Amarillis

I used this just as an example. On my real program that motivated this I'm trying to create a wrap for a C library called igraph, and my class have a pointer to an igraph variable. – Sentience

@PaulGroke regarding your statement: "That means you need to implement copy-and-swap again in the new class to get 'all or nothing' assignment". Would you provide your own assignment operator if its sole purpose was to ensure atomic assignment, i.e. for a class which is not managing any resources so shallow copying is not an issue but the class does contain 2 or more data members which could potentially throw on memberwise assignment. – Pomposity

@Skeve: Without going into great lengths, the best answer is "it depends" :-) – Intercostal

@PaulGroke When you say "it depends" do you mean this in the sense of this question and answer or something else? I am really interested in people's thoughts on this, perhaps I should post it as a separate question. – Pomposity

@Skeve: I mean if it's important to e.g. keep invariants, and you want or need to have a robust class that keeps functional even after throwing an exception from the assignment operator, then of course you want to make it all-or-nothing somehow - be it via copy & swap or some other technique. OTOH if it's not really important and could hurt performance, then you might not want to. – Intercostal

Answer 1

1 - Are the methods and constructors as implemented above for the Bar class safe? Having used the copy-and-swap for Foo make me sure that no harm can be done when assigning or copying Bar?

Regarding the copy-ctor: that's always safe (all-or-nothing). It either completes (all), or it throws an exception (nothing).

If your class is only made up of one member (i.e. no base classes as well), the assignment operator will be just as safe as that of the member's class. If you have more then one member, the assignment operator will no longer be "all or nothing". The second member's assignment operator could throw an exception, and in that case the object will be assigned "half way". That means you need to implement copy-and-swap again in the new class to get "all or nothing" assignment.

However it will still be "safe", in the sense that you won't leak any resources. And of course the state of each member individually will be consistent - just the state of the new class won't be consistent, because one member was assigned and the other was not.

2 - Passing the argument by reference in the copy constructor and in swap is mandatory?

Yes, passing by reference is mandatory. The copy constructor is the one that copies objects, so it cannot take it's argument by value, since that would mean the argument has to be copied. This would lead to infinite recursion. (The copy-ctor would be called for the argument, which would mean calling the copy-ctor for the argument, which would mean ...). For swap, the reason is another: if you were to pass the argument by value, you could never use swap to really exchange the contents of two objects - the "target" of the swap would be a copy of the object originally passed in, which would be destroyed immediately.

3 - Is it right to say that when the argument of operator= is passed by value, the copy constructor is called for this argument to generate a temporary copy of the object, and that it is this copy which is then swapped with *this? If I passed by reference in operator= I would have a big problem, right?

Yes, that's right. However it's also quite common to take the argument by reference-to-const, construct a local copy and then swap with the local copy. The by reference-to-const way has some drawbacks however (it disables some optimizations). If you're not implementing copy-and-swap though, you should probably pass by reference-to-const.

4 - Are there situations in which this idiom fails to provide complete safety in copying and assigning Foo?

None that I know of. Of course one can always make things fail with multi-threading (if not properly synchronized), but that should be obvious.

Intercostal

#1 -- not true, if an exception is thrown then memory may be leaked. Use a function-try-block to ensure that foo is deleted in case of exception. – Booker

Nope. The only thing in Foo::Foo that can throw is the new, and if it does, the memory will not have been allocated, so there's nothing to clean up. If it was something other than a bunch of ints, the copy might throw, and one would have to protect against that. In that case however I'd suggest to use an additional base-class that just allocates & owns the array, without doing any copying in it's own ctor. That way you don't need a function try block. – Intercostal

There's a function call. Absent additional information about the function (is it std::copy? Probably not, since std:: is explicitly provided when calling std::swap), one must assume it can throw. – Booker

You're splitting the hair mighty thin here. Yes, if copy() could throw, then Foo::Foo would be broken. Still the question isn't about Foo::Foo, it's all about Bar. And assuming Foo::Foo is OK, so will be Bar's implementation. And of course assuming Foo is broken in some way, so must be Bar. – Intercostal

Ahh, yes. The Bar constructor is fine. +1 – Booker

Answer 2

As much as possible, you should initialize the members of your class in the initializer list. That will also take care of the bug I told you about in the comment. With this in mind, your code becomes:

class Foo {
private:
  int size;
  int * foo;

public:
  Foo(size_t size) : size(size), foo(new int[size]) {}
  ~Foo(){delete[] foo;} // note operator delete[], not delete

  Foo(Foo const& other) : size(other.size), foo(new int[other.size]) {
    copy(other.foo, other.foo + size, foo);
  }

  Foo& swap(Foo& other) { 
    std::swap(foo,  other.foo);  
    std::swap(size, other.size); 
    return *this;
  }

  Foo& operator=(Foo g) { 
    return swap(g); 
  }

  int& operator[] (const int idx) {return foo[idx];}
};

and

class Bar {
private:
  Foo bar;
public:
  Bar(Foo foo) : bar(foo) {};
  ~Bar(){};
  Bar(Bar const& other) : bar(other.bar) { }
  Bar& swap(Bar &other) { bar.swap(other.bar); return *this; }
  Bar& operator=(Bar other) { return swap(other); }
}

which uses the same idiom throughout

note

as mentioned in a comment on the question, Bar's custom copy constructors etc. are unnecessary, but we'll assume Bar has other things as well :-)

second question

Passing by reference to swap is needed because both instances are changed.

Passing by reference to the copy constructor is needed because if passing by value, you'd need to call the copy constructor

third question

yes

fourth question

no, but it is not always the most efficient way of doing things

Rahal

So, that's kind one of the points I was doubtful about: is it completely safe to trust in the default copy constructor and assignment operator for the class Bar as it is? – Sentience

@Rafael yes, it is. Both Bar's default copy constructor and its default assignment operator will work just fine – Rahal

@Rafael: the compiler-provided default constructors/assignment operator/destructor are fine as long as the class members and bases are properly implemented with "value semantics", which means they manage the resources they use. If Bar contained e.g. raw pointers to heap-allocated memory, then 1) you could write your own Bar constructors/destructor/operator= to manage that so Bar still had value semantics (good), or 2) you could have the Foo clean up after Bar (error prone, and requires Bar-related steps in Foo's constructors/destructor/operator=). – Richman

Answer 3

This is a classic example of where following an idiom leads to unnecessary performance penalties (premature pessimization). This isn't your fault. The copy-and-swap idiom is way over-hyped. It is a good idiom. But it should not be followed blindly.

Note: One of the most expensive things you can do on a computer is allocate and deallocate memory. It is worthwhile to avoid doing so when practical.

Note: In your example, the copy-and-swap idiom always performs one deallocation, and often (when the rhs of the assignment is an lvalue) one allocation as well.

Observation: When size() == rhs.size(), no deallocation or allocation need be done. All you have to do is a copy. This is much, much faster.

Foo& operator=(const Foo& g) {
    if (size != g.size)
        Foo(g).swap(*this); 
    else
       copy(other.foo, other.foo + size, foo);
    return *this; 
}

I.e. check for the case where you can recycle resources first. Then copy-and-swap (or whatever) if you can't recycle your resources.

Note: My comments do not contradict the good answers given by others, nor any correctness issues. My comment is only about a significant performance penalty.

Koehler

What you're saying is quite correct. However, you should add an rvalue-reference overload. Otherwise your "optimization" will in fact hurt performance whenever an rvalue is used on the right side of the assignment. – Intercostal

@pgroke: rvalues references are only available in C++0x, I don't think the OP is quite ready and I am afraid it would only confuse him (bear in mind that he's unsure of how references work). I am afraid this answer is already a bit too far-fetched as it is. – Amarillis

I know rvalue references are only available in C++0x. You may be right, discussing rvalue references could confuse the OP. Nevertheless, I think if something like this optimization is discussed at all, it can't hurt to be as complete as possible. – Intercostal

Problem with this "optimization" -- the object is left in a mess if element copy constructors can throw (not really a problem for int). copy-and-swap either succeeds or leaves the old value intact. – Booker