-1

Trying to resolve error C2248 related to abstract base class using implementation of copy/move ctors/assignment operators and dtor (Rule of Five) and a few questions come up:

1) Why does the rule of 5, primarily relating to the dtor, apply when the unique_ptr data members are handled automatically? The dtor implementation should be left empty correct, since the unique_ptrs are automatically destroyed once their owners go out of scope?

2) Suppose another class had a member of type std::unique_ptr of a vector of the same type. In order for this class to be copyable, it must have a copy ctor and copy assignment operator that clone the unique_ptr data member? I have seen this solution, but is seems like the original poster just switched over to shared_ptr for the sake of removing the error alone with little consideration of ownership management. Is this the correct strategy?

3) Consider the same case as question 2 above relating to vector of unique_ptr. Should dtor include a call to clear() the vector?

4) The assignment operators for the Derived1 are not correct. But the base class is supposed to have copy and move assignment operators, since it has copy/move ctors (rule of 4/5). These can't actually be used outside of the class since it is abstract and thus no instances will be assigned. But how do I utilize this code from the derived classes? Each derived class needs to be able to move/copy the base data members and it's own data members. I'm not sure what to do.

    #include <algorithm>
#include <memory>
#include <vector>
#include <iostream>

class Base{

public:
    Base() : m_subBases(){};

    /*  copy ctor */
    Base(const Base& other) : m_subBases(){
        *this = other;
    };

    /*  move ctor */
    Base(Base&& other) : m_subBases(){
        *this =std::move( other);
    };

    /*  Move assignment operator*/
    Base& operator=(Base&& other){
        m_subBases = std::move(other.m_subBases);
        return *this;
    };

    /*  Copy assignment operator */
    Base& operator=(const Base& other){
        for(int i = 0; i < other.m_subBases.size(); i++)
            m_subBases.push_back(other.m_subBases[i]->clone());

        return *this;
    };

    /* virtual dtor */
    virtual ~Base(){
        m_subBases.clear();
    };

    /* Used for creating clones of unique_ptrs */
    virtual std::unique_ptr <Base> clone() const= 0;

    /* Do something */
    virtual void execute(float f) = 0;

    //Omitted data member access methods

protected:
    std::vector < std::unique_ptr <Base> > m_subBases;
};

class Derived1 : public Base{

public:
    Derived1() :  Base(){};

    /*  copy ctor */
    Derived1(const Derived1& other) : Base(other){
        *this = other;
    };

    /*  move ctor */
    Derived1(Derived1&& other) : Base(std::move(other)){
        *this = std::move(other);
    };

    /*  Move assignment operator*/
    Derived1& operator=(Derived1&& other){

        //This is redundant when called in the move ctor because
        // of the call to Base(std::move(other))
        m_subBases = std::move(other.m_subBases);

        m_string = other.m_string;
        return *this;
    };

    /*  Copy assignment operator */
    Derived1& operator=( const Derived1& other){

        //This is redundant when called in the copy ctor because
        // of the call to Base(other)
        for(int i = 0; i < other.m_subBases.size(); i++)
            m_subBases.push_back(other.m_subBases[i]->clone());

        m_string = other.m_string;
        return *this;
    };

    /* virtual dtor */
    virtual ~Derived1(){};

    /* Used for creating clones of unique_ptrs */
    virtual std::unique_ptr <Base> clone() const{
        return std::unique_ptr <Base> (new Derived1(*this));
    };

    virtual void execute(float f){
        std::cout << "Derived1 " << f << std::endl; 
    };
protected:

    std::string m_string;
};
Community
  • 1
  • 1
  • 3
    Look! A wall of code with a wall of text! – Tony The Lion Sep 18 '12 at 15:49
  • 2
    Yes, please create a short example that shows your problem, with a short explanation. It's just overwhelming to read this. – Tony The Lion Sep 18 '12 at 15:50
  • 3
    Yes. Please edit your question to make it short and concise. For example, sentences like ***"I hope the title question to this post accurately reflects the nature of the problem."*** does NOT add any valuable information related to the problem. Please remove all such sentences from the question. – Nawaz Sep 18 '12 at 15:51
  • Sorry, trying to err on giving too much information rather than too little. Removed second derived class, but other code seems justified since trying to adhere to rule of five? – user1679238 Sep 18 '12 at 15:54
  • 2
    Assignment in the copy constructor is very frightening. – Kerrek SB Sep 18 '12 at 15:55
  • 2
    I think the important message to take home is that when you're using components like `unique_ptr`, you *don't* write any of the copy constructors, copy-assignment operators and destructors. The implicitly defined versions will do just fine. – Kerrek SB Sep 18 '12 at 16:01
  • 1
    I would like to mention the rule of zero at this point. – Tony The Lion Sep 18 '12 at 16:01
  • I would argue that there isn't really a rule of five (or four or whatever) in C++11. Due to the existance of moveable only types it is perfectly valid to have classes with custom copy constructor/assignment, but default destructor and move constructor/assignment, classes where of the five only the destructor is default, classes where you don't have copy operations and so on. For example your custom move operations for `Base` are not necessary, since they only do what the default does anyways. – Grizzly Sep 18 '12 at 16:01
  • @KerrekSB: Shouldn't that be don't write move constructors/assignment and destructors? Because I can't see how the default copy operations would work with `unique_ptr`s. – Grizzly Sep 18 '12 at 16:03
  • @KerrekSB Thank you, I think I see now why from [this](http://msdn.microsoft.com/en-us/magazine/cc163742.aspx). Is it appropriate to do so in the move constructor, as in this [link](http://msdn.microsoft.com/en-us/library/dd293665.aspx) under "Robust Programming"? – user1679238 Sep 18 '12 at 16:04
  • @KerrekSB When you're using `unique_ptr`, the implicitly defined copy constructor and assignment operator likely don't do just fine. _If_ you're supporting copy, and the pointed to object is part of your value representation, you probably want a deep copy of the pointed to object (or you want `shared_ptr`, rather than `unique_ptr`). And of course, if you have more than one such pointer, and need transactional integrity, even a deep copying pointer won't suffice. – James Kanze Sep 18 '12 at 16:08
  • @user1679238: No, never! If anything (but don't): *Do* write a copy/move constructor. *Do* write a swap function. Then implement assignment as a single function, `Foo & operator=(Foo rhs) { swap(rhs); return *this; }`. – Kerrek SB Sep 18 '12 at 16:09
  • @JamesKanze: Sorry, I was speaking in a generalized mode. When you have a `unique_ptr` member, your class should *not* have copy semantics, only move semantics. If you want deep copying, don't reinvent the wheel, but use an appropriate [`value_ptr` class](http://hg.tumtumtree.me/wheels/src/c8e1b8979492/include/wheels/smart_ptr/value_ptr.h%2B%2B). Then proceed as per my earlier comment. – Kerrek SB Sep 18 '12 at 16:10
  • @KerrekSB So if Base should not have copy semantics since it has a unique_ptr, then any other class that has a unique_ptr instance of Base's subclasses should also be non-copyable? This was the very first issue that started all of this. I thought I had taken care of move semantics, but each subsequent class that had anything to do with the Base-like class was throwing error C2248. – user1679238 Sep 18 '12 at 16:22
  • @user1679238: It'd be much easier if you encapsulated the deep-copy semantics into a once-and-for all smart pointer and *not* try to reinvent the wheel at every single step in your code! – Kerrek SB Sep 18 '12 at 16:25
  • Wow, there's a lot of noise in this question. This is really 4 or 5 questions all smushed together and made less clear in the process. In the future, try asking your questions individually and make sure each one is clear and concise. – Phil Hord Sep 18 '12 at 19:14

1 Answers1

2

I'd like to offer an alternative approach. Not the Scary Rule of Five, but the Pleasant Rule of Zero, as @Tony The Lion has already suggested. A full implementation of my proposal has been coded by se­ve­ral people, and there's a fine version in @R. Martinho Fernandes's library, but I'll present a simplified version.

First, let's recap:

The Rule of Zero: Don't write a copy- or move-constructor, a copy- or move-assignment ope­ra­tor, or a destructor. Instead, compose your class of components which handle a single responsibility and encap­su­late the desired behaviour for the individual resource in question.

There's an obvious caveat: When you design the single-responsibility class, you must of course obey:

The Rule of Five: If you write any one of copy- or move-constructor, copy- or move-assignment ope­ra­tor, or destructor, you must implement all five. (But the "five" functions needed by this rule are actually: Destructor, Copy-Const, Move-Const, Assignment and Swap.)

Let's do it. First, your consumer:

struct X;

struct Base
{
    std::vector<value_ptr<X>> v;
};

struct Derived : Base
{
};

Note that both Base and Derived obey the Rule of Zero!

All we need to do is implement value_ptr. If the pointee is non-polymorphic, the following will do:

template <typename T>
class value_ptr
{
    T * ptr;
public:
    // Constructors
    constexpr value_ptr()      noexcept : ptr(nullptr) { }
    constexpr value_ptr(T * p) noexcept : ptr(p)       { }

    // Rule of Five begins here:
    ~value_ptr() { ::delete ptr; }
    value_ptr(value_ptr const & rhs) : ptr(rhs.ptr ? ::new T(*rhs.ptr) : nullptr) { }
    value_ptr(value_ptr && rhs) noexcept : ptr(rhs.ptr) { rhs.ptr = nullptr; }
    value_ptr & operator=(value_ptr rhs) { swap(rhs); return *this; }
    void swap(value_ptr & rhs) noexcept { std::swap(rhs.ptr, ptr); }

    // Pointer stuff
    T & operator*() const noexcept { return *ptr; }
    T * operator->() const noexcept { return ptr; }
};

template <typename T, typename ...Args>
value_ptr<T> make_value(Args &&... args)
{
    return value_ptr<T>(::new T(std::forward<Args>(args)...));
}

If you would like smart pointer that handles polymorphic base class pointers, I suggest you demand that your base class provide a virtual clone() function, and that you implement a clone_ptr<T>, whose copy constructor would be like this:

clone_ptr(clone_ptr const & rhs) : ptr(rhs.ptr ? rhs.ptr->clone() : nullptr) { }
Kerrek SB
  • 464,522
  • 92
  • 875
  • 1,084
  • Why would you need to implement all five? Whats wrong with implementing copy operations yourself while leaving the rest defaulted, for responsability classes containing move only objects? – Grizzly Sep 18 '12 at 16:38
  • @Grizzly: If all other functions can be left defaulted, then it stands to reason that your copy constructor can also be left defaulted. Which ever way you turn it, if you really do need to define any one of the five, you are very likely doing some manual resource handling that needs to be handled in all of them. – Kerrek SB Sep 18 '12 at 16:42
  • What I mentioned was a scenario where the class contains a member variable, which is movable, but not copieable. That is hardly doing manual resource handling. In that case the default is typically fine for the destructor and move operations, since those will be handled by the member, however copying needs special attention, since the member doesn't have copy capabilities, so depending on what you are doing you either deep copy or create a new object. Another scenario would be a member with neither copy or move functionality, where the default destructor would still be fine, but nothing else. – Grizzly Sep 18 '12 at 16:50
  • 1
    @Grizzly: Well, I'm sure you can contrive a situation like the one you describe, but I'd like to claim that you could almost always handle this more elegantly by using an appropriate component, rather than, say, abusing a unique pointer for a deep-copying role. The Rule of Five is a general design guide, not an absolute mandate. If you follow it, your code will be nicer, but you don't *have* to. – Kerrek SB Sep 18 '12 at 16:52
  • @KerrekSB Trying to play with code. Reviewing this [link](http://wiki.apache.org/stdcxx/C%2B%2B0xCompilerSupport), VS 2010 does not support constexpr, noexcept nor variadic templates. I don't think constexpr is essential for functionality, nor is the make_ function since the ctor can be used? But how significant is the use of noexcept in this example? – user1679238 Sep 18 '12 at 18:43
  • @user1679238: Feel free to throw all that stuff out. I just put it in because it's a nice touch, but it's by no means essential. – Kerrek SB Sep 18 '12 at 19:35
  • @KerrekSB Thanks for the advice! I never would have thought to use templates. – user1679238 Sep 18 '12 at 21:25