What is the proper approach to swap and copy idiom in virtual inheritance?

Question

Consider classic virtual inheritance diamond hierarchy. I wonder to know what is the right implementation of copy and swap idiom in such hierarchy.

The example is a little artificial - and it is not very smart - as it would play good with default copy semantic for A,B,D classes. But just to illustrate the problem - please forget about the example weaknesses and provide the solution.

So I have class D derived from 2 base classes (B<1>,B<2>) - each of B classes inherits virtually from A class. Each class has non trivial copy semantics with using of copy and swap idiom. The most derived D class has problem with using this idiom. When it calls B<1> and B<2> swap methods - it swaps virtual base class members twice - so A subobject remains unchanged!!!

A:

class A {
public:
  A(const char* s) : s(s) {}
  A(const A& o) : s(o.s) {}
  A& operator = (A o)
  {
     swap(o);
     return *this;
  }
  virtual ~A() {}
  void swap(A& o)
  {
     s.swap(o.s);
  }
  friend std::ostream& operator << (std::ostream& os, const A& a) { return os << a.s; }

private:
  S s;
};

B

template <int N>
class B : public virtual A {
public:
  B(const char* sA, const char* s) : A(sA), s(s) {}
  B(const B& o) : A(o), s(o.s) {}
  B& operator = (B o)
  {
     swap(o);
     return *this;
  }
  virtual ~B() {}
  void swap(B& o)
  {
     A::swap(o);
     s.swap(o.s);
  }
  friend std::ostream& operator << (std::ostream& os, const B& b) 
  { return os << (const A&)b << ',' << b.s; }

private:
  S s;
};

D:

class D : public B<1>, public B<2> {
public:
  D(const char* sA, const char* sB1, const char* sB2, const char* s) 
   : A(sA), B<1>(sA, sB1), B<2>(sA, sB2), s(s) 
  {}
  D(const D& o) : A(o), B<1>(o), B<2>(o), s(o.s) {}
  D& operator = (D o)
  {
     swap(o);
     return *this;
  }
  virtual ~D() {}
  void swap(D& o)
  {
     B<1>::swap(o); // calls A::swap(o); A::s changed to o.s
     B<2>::swap(o); // calls A::swap(o); A::s returned to original value...
     s.swap(o.s);
  }
  friend std::ostream& operator << (std::ostream& os, const D& d) 
  { 
     // prints A::s twice...
     return os 
    << (const B<1>&)d << ',' 
    << (const B<2>&)d << ',' 
        << d.s;
  }
private:
  S s;
};

S is just a class storing string.

When doing copy you will see A::s remains unchanged:

int main() {
   D x("ax", "b1x", "b2x", "x");
   D y("ay", "b1y", "b2y", "y");
   std::cout << x << "\n" << y << "\n";
   x = y;
   std::cout << x << "\n" << y << "\n";
}

And the result is:

ax,b1x,ax,b2x,x
ay,b1y,ay,b2y,y
ax,b1y,ax,b2y,y
ay,b1y,ay,b2y,y

Probably adding B<N>::swapOnlyMewould resolve the problem:

void B<N>::swapOnlyMe(B<N>& b) { std::swap(s, b.s); }
void D::swap(D& d) { A::swap(d); B<1>::swapOnlyMe((B<1>&)d); B<2>::swapOnlyMe((B<2>&)d); ... }

But what when B inherits privately from A?

Sheehy

Answer 1

Here's a philosophical rant:

1. I don't think virtual inheritance can or should be private. The entire point of a virtual base is that the most derived class owns the virtual base, and not the intermediate classes. Thus no intermediate class should be permitted to "hog" the virtual base.

2. Let me repeat the point: The most derived class owns the virtual base. This is evident in constructor initializers:

   D::D() : A(), B(), C() { }
   //       ^^^^
   //       D calls the virtual base constructor!
   

   In the same sense, all other operations in D should be immediately responsible for A. Thus we are naturally led to writing the derived swap function like this:

   void D::swap(D & rhs)
   {
       A::swap(rhs);   // D calls this directly!
       B::swap(rhs);
       C::swap(rhs);
   
       // swap members
   }
   

3. Putting all this together, we're left with only one possible conclusion: You have to write the swap functions of the intermediate classes without swapping the base:

   void B::swap(B & rhs)
   {
       // swap members only!
   }
   
   void C::swap(C & rhs)
   {
       // swap members only!
   }
   

Now you ask, "what if someone else wants to derive from D? Now we see the reason for Scott Meyer's advice to always make non-leaf classes abstract: Following that advice, you only implement the final swap function which calls the virtual base swap in the concrete, leaf classes.

---

Update: Here's something only tangentially related: Virtual swapping. We continue to assume that all non-leaf classes are abstract. First off, we put the following "virtual swap function" into every base class (virtual or not):

struct A
{
    virtual void vswap(A &) = 0;
    // ...
};

Usage of this function is of course reserved only to identical types. This is safeguarded by an implicit exception:

struct D : /* inherit */
{
    virtual void vswap(A & rhs) { swap(dynamic_cast<D &>(rhs)); }

    // rest as before
};

The overall utility of this is limited, but it does allow us swap to objects polymorphically if we happen to know that they're the same:

std::unique_ptr<A> p1 = make_unique<D>(), p2 = make_unique<D>();
p1->vswap(*p2);

Obduliaobdurate

While I agree with most of your answer (+1), I am not sure that I understand the last sentence. Do you mean that swap() should only be implemented in D, but not in A or B<N>? This may well be impossible due to data encapsulation in the base classes. Besides, and I admit that I have not tested the code, it looks like your D::swap() implementation would work correctly even with "normal" swap() in the base classes, since C::swap() would cancel the A part of B::swap(). – Magocsi

@Gorpik: Only the final-swap should go into the leaf. The final-swap is the one which swaps the virtual base. Everyone else as a swap, too, but one that doesn't touch the base. (And think about what happens if you have an odd number of base classes.) – Obduliaobdurate

@Gorpik: It was my first approach with direct calling A::swap from D, and with 3 base classes B<1>,B<2>,B<3> I had wrong behavior of D::swap. – Sheehy

@KerrekSB (+1) but I have still additional questions: Non-leaf classes in one hierarchy could be leaf classes in other places. And what about non virtual inheritance? I guess some C::swap shall swaps all theirs members and their non-virtual direct base classes members? And what with direct call to B<N>::swap? No-one expect that B<1> b1, b2; b1.swap(b2); does not swap virtual base classes - so I guess two swap methods are necessary? But if I got the B<N> from elsewhere - it can have only one swap()? – Sheehy

@PiotrNycz: First off, non-virtual bases don't add any complexity, since you just treat them like you always have. My point about leaves is that you should have enough control over your class hierarchy to know whether something is a leaf or not, and make that decision definitively. If you are willing to introduce virtual bases into your hierarchy, you become responsible to some extent for the final class. – Obduliaobdurate

@KerrekSB: OK, now I understand your answer and I agree with the last line too. I had also thought of an odd number of intermediate base classes, but honestly, I thought you deserve any bad things happening to you if you add one more edge to the diamond. – Magocsi

@KerrekSB Sometimes classes are from third party library and they are non abstract classes. Maybe proper answer would be - always add to such classes as B<N> protected function swapNonVirtual() and if it is non-abstract class then add also public swap() function? – Sheehy

@PiotrNycz: I'd say, simply don't derive from third-party library classes! And if a third party mandates that you derive from them, they would surely document in detail how you're supposed to use the base. – Obduliaobdurate

@KerrekSB - thanks, it is clear now. I'll probably accept your answer - just waiting a day to let other chance to compete... – Sheehy

@PiotrNycz: No worries, and no pressure. This is the first time I've thought about swapping bases, and it's been fun. You're more than welcome to accept any other answer you prefer! – Obduliaobdurate

Presuming I accept your design (and I've never done this one either), like the qestioner I wouldn't call the functions B::swap and C::swap. Obviously you can give any name you like to anything you like, but since they aren't "complete" swap operations I'd call them swap_impl or something just to make it clear that there's something clever going on. And maybe have a pure virtual swap to make it clear that the intermediate class is not capable of swapping a complete object, or a separate swap that also swaps A if I insist on committing the sin of writing a non-abstract base class. – Terracotta

@SteveJessop: I had originally wanted to call them vswap, but after I thought about it some more, I decided that you can't have any ambiguity if you follow my suggestions: A class cannot both have a direct virtual base and be a leaf. – Obduliaobdurate

I've reused the name vswap for a genuinely virtual swap function and outlined its use. – Obduliaobdurate

Thanks for the update. The answer really teached me new things. – Sheehy

@PiotrNycz: No problem -- me too! First time I thought about swapping derived classes :-) – Obduliaobdurate

Answer 2

Virtual base generally means that most derived class of object is in control of it.

First solution: Reorganize your classes to be more fit for polymorphism. Make copy construction protected. Remove assignment and swap(). Add virtual clone(). Idea is that the classes should be treated as polymorphic. So they should be used with pointer or smart pointer. Swapped or assigned should be the pointer values, not the object values. In such context swap and assignment only confuse.

Second solution: Make B and C abstract and their pointers not to manage object lifetime. Destructors of B and C should be protected and non-virtual. B and C will not therefore be most derived classes of object. Make B::swap() and C::swap() protected and not swap the A subobject, may rename or add comment that it is business of inherited classes now. That removes lot of object slicing possibilities. Make D::swap() to swap A subobject. You get one swap of A.

Third solution: Make D::swap() to swap A subobject. That way the A subobject will be swapped 3 times and lands in correct place. Inefficient? The whole construct is probably bad idea anyway. I am for example not sure how well the virtual destructors and swaps cooperate here and lot of ways to slice objects are public here. It all seems similar to attempt of making virtual assignment operators that is bad idea in C++.

If something inherits from D on its order then it should make sure by swapping or not swapping A subobject that the count of swapping A is odd. It becomes to control so should take over and fix.

The private virtual idiom is one of ways to make a class final in C++. Nothing should be able to inherit from it. Interesting that you asked. If you ever use it, be sure to comment, it confuses most of the readers of code.

Krona

What iF I add third base class B<3> - then it is not working again. The problem is not with calling A::swap odd times - but with how to achieve to call it exactly once. And what if B and C works as most derived objects? – Sheehy

Most of your answer is already in Kerrek SB's one, but the last paragraph is interesting, thanks for it. – Magocsi

@Sheehy 1) it is business of every class to resolve the issues with its virtual bases. So if you have third base class of D then it should stop swapping A again. 2) When B and C are abstract then they can not be most derived objects and can leave the issues with virtual bases to derived classes. – Mukden

+1 and thanks. I accepted Kerrek's answer. I'd accept yours if I ask sth like "is it wise to make class using virtual inheritance assignable". – Sheehy

Wait, what is the private virtual idiom? I know a "non-virtual interface" idiom, but that doesn't prevent inheritance. – He

@MooingDuck It is pre-C++11 way to make a class final by private virtual inheritance. Derived class does not have access to base of base that it must construct and that results with compiling error. – Mukden