tags:

views:

214

answers:

4

I want to add swap functionality to two existing C++ classes. One class inherits from the other. I want each classes' instances to only be swappable with instances of the same class. To make it semi-concrete, say I have classes Foo and Bar. Bar inherits from Foo. I define Foo::swap(Foo&) and Bar::swap(Bar&). Bar::swap delegates to Foo::swap. I want Foo::swap to only work on Foo instances and Bar::swap to only work on Bar instances: I can't figure out how to enforce this requirement.

Here's a sample of what's giving me trouble:

#include <algorithm>
#include <iostream>

struct Foo {
    int x;
    Foo(int x) : x(x) {};

    virtual void swap(Foo &other) {
        std::cout << __PRETTY_FUNCTION__ << std::endl;
        std::swap(this->x, other.x);
    };
};

struct Bar : public Foo {
    int y;
    Bar(int x, int y) : Foo(x), y(y) {};

    virtual void swap(Bar &other) {
        std::cout << __PRETTY_FUNCTION__ << " ";
        Foo::swap(other);
        std::swap(this->y, other.y);
    };
};

void display(Foo &f1, Foo &f2, Bar &b34, Bar &b56)
{
    using namespace std;

    cout << "f1:  " << f1.x                  << endl;
    cout << "f2:  " << f2.x                  << endl;
    cout << "b34: " << b34.x << " " << b34.y << endl;
    cout << "b56: " << b56.x << " " << b56.y << endl;
}

int main(int argc, char **argv)
{
    {
        Foo f1(1), f2(2);
        Bar b34(3,4), b56(5,6);
        std::cout << std::endl << "Initial values: " << std::endl;
        display(f1,f2,b34,b56);
    }

    {
        Foo f1(1), f2(2);
        Bar b34(3,4), b56(5,6);
        std::cout << std::endl << "After Homogeneous Swap: " << std::endl;
        f1.swap(f2);             // Desired
        b34.swap(b56);           // Desired
        display(f1,f2,b34,b56);
    }

    {
        Foo f1(1), f2(2);
        Bar b34(3,4), b56(5,6);
        std::cout << std::endl << "After Heterogeneous Member Swap: " << std::endl;
        // b56.swap(f2);         // Doesn't compile, excellent
        f1.swap(b34);            // Want this to not compile, but unsure how
        display(f1,f2,b34,b56);
    }

    return 0;
}

Here's the output:

Initial values: 
f1:  1
f2:  2
b34: 3 4
b56: 5 6

After Homogeneous Swap: 
virtual void Foo::swap(Foo&)
virtual void Bar::swap(Bar&) virtual void Foo::swap(Foo&)
f1:  2
f2:  1
b34: 5 6
b56: 3 4

After Heterogeneous Member Swap: 
virtual void Foo::swap(Foo&)
f1:  3
f2:  2
b34: 1 4
b56: 5 6

You can see in the final output group where f1.swap(b34) "sliced" b34 in a potentially nasty way. I'd like the guilty line to either not compile or blow up at runtime. Because of the inheritance involved, I think I run into the same problem if I use a nonmember or friend swap implementation.

The code is available at codepad if that helps.

This use case arises because I want to add swap to boost::multi_array and boost::multi_array_ref. multi_array inherits from multi_array_ref. It only makes sense to swap multi_arrays with multi_arrays and multi_array_refs with multi_array_refs.

+1  A: 

You cannot really do this, but I don't see the point, anyway. It's no worse than slicing on operator= or copy constructors, and you cannot avoid the latter, either. Why should swap be any different?

For the same reason, it's likely not worth it making swap virtual, for the same reason why you don't make operator= virtual.

Pavel Minaev
It hadn't occurred to me that it's a standard slicing problem present in many other places. I'm much less worried about it now since people's hackles may already be raised about this class of problem.
Rhys Ulerich
+4  A: 

Swap, like assignment and comparison work well with value types and don't work well with bases of class hierarchies.

I've always found it easiest to follow the Scott Meyer's Effective C++ recommendation of not deriving from concrete classes and making only leaf classes concrete. You can then safely implement swap, operator==, etc. as non-virtual functions for leaf nodes only.

While it's possible to have a virtual swap functions the whole point of having virtual base classes is to have dynamic behaviour at runtime so I think you're on to a loser trying to get all incorrect possibilities to fail at compile time.

If you want to go the virtual swap route, then one possible approach is to do something like this.

class Base
{
public:
    virtual void Swap(Base& other) = 0;
};

class ConcreteDerived
{
    virtual void Swap(Base& other)
    {
        // might throw bad_cast, in this case desirable
        ConcreteDerived& cother = dynamic_cast<ConcreteDerived&>(other);bad_cast
        PrivateSwap(cother);
    }

    void PrivateSwap(ConcreteDerived& other)
    {
        // swap implementation
    }
};
Charles Bailey
Thank you for the suggestion, Charles. I need Base::Swap to be non-virtual because of my use case. I'm not sure I can adopt this since the dynamic cast used in a Base::Swap similar to ConcreteDerived::Swap would always succeed.
Rhys Ulerich
If your base class Swap is non-virtual then you will have little to no protection against slicing. Any class that you add to the hierarchy will have a default swap action that will slice. Without some sort of polymorphic behaviour you are not going to be able to successfully swap two derived classes through pointers or reference to their base class. A virtual function would be the most natural way to do this.
Charles Bailey
+3  A: 

(Somewhat hacky solution)

Add a protected virtual method, isBaseFoo(), make it return true in Foo, and false in Bar, the the swap method for Foo could check it's argument has isBaseFoo()==true.

Evil, and detects the problem only at run-time, but I can't think of anything better, although Charles Bailey's answer might be better, if you allow dynamic_cast<>.

Douglas Leeder
Definitely ugly, but I think this is the only thing that's workable in practice. Thanks Douglas.
Rhys Ulerich
A: 

What you are actually trying to do is swap instances of classes from a third party inheritance hierarchy. Given that, I'd forget about using swap on the actual classes and add a level of indirection. Using boost::shared_ptr is a good approach; use shared_ptr instances containing whatever class you want, and swap to your heart's content.

In general, solving the problem as you asked it at compile time is hard for all the reasons described by the other answerers.

janm