Is this the right way to implement pimpl wth unique_ptr and move-semantics in C++11

Question

I haven't yet seen a pimpl example that utilizes both unique_ptr and move-semantics.

I want to add a CHelper class to STL derived containers and use pimpl to hide what CHelper does.

Does this look right?

Derived.h

class CDerived : public set<CSomeSharedPtr>, public CHelper  
{
//...
};

`

Helper.h

// derived containers need to support both copy and move, so CHelper does too  

class CHelper  
{  
private:  
    class impl;  
    unique_ptr<impl> pimpl;  

public:  
//--- default: need both cotr & cotr (complete class) in order to use unique_ptr<impl>  
    CHelper();  
    ~CHelper();  

//--- copy  
    CHelper(const CHelper &src);         //copy constructor  
    CHelper& operator=(const CHelper &src);//assignment operator  

//--- move  
    CHelper(CHelper &&src);         //move constructor  
    CHelper& operator=(CHelper &&src);//move operator  

//--- expose public methods here  
    void SetModified(BOOL bSet=TRUE);  
};  

Helper.cpp

//===========================  
class CHelper::impl  
{  
public:  
BOOL m_bModified; //has the container been modified (needs to be saved)  
// ... other data  

impl() {m_bModified = FALSE;}  

//--- copy cotr/assign  
impl(const impl &src)  
{  
  *this = src;  
}  

void operator=(const impl &src)   
{  
  m_bModified = src.m_bModified;  
  // ...other data  
}  

//--- move cotr/assign ?? do I need to write move cotr/assign ??   

};  

//============================  
CHelper::CHelper() : pimpl(unique_ptr<impl>(new impl())) {}

CHelper::~CHelper() {}  

//--- copy  
CHelper::CHelper(const CHelper &src)  
: pimpl(unique_ptr<impl>(new impl(*src.pimpl))) {}

CHelper& CHelper::operator=(const CHelper &src)  
{  
  if (this != &src)  
    *pimpl = *src.pimpl;  

  return *this;  
}  

//--- move  
CHelper::CHelper(CHelper &&src)  
{  
  if (this != &src)  
  {  
    pimpl = move(src.pimpl); //use move for unique_ptr  
    src.pimpl.reset(nullptr);//leave pimpl in defined / destructable state  
  }  
}  

CHelper& CHelper::operator=(CHelper &&src)  
{  
    if (this != &src)  
    {  
      pimpl = move(src.pimpl); //use 'move' for unique_ptr  
      src.pimpl.reset(nullptr);//leave pimpl in defined / destructable state  
    }  
    return *this;  
}  

Answer 1

Considering that the only member of CHelper is a unique_ptr and that the default implementation of copy is calling the copy of the bases and of the members, the default implementation for move is calling the move of the bases and members, there is no need to override the CHelper ctors and assigns. Just let the default do their job. They'll just call the appropriate unique_ptr move constructor and operator.

About putting together CHelper and a set<...> to form a CDerived... that's not a "canonical design" (set is not a OOP class..., and CHelper also isn't) but can work if used properly (don't try to assign a CDerived to a CHelper* ad call delete on it, or you will end in tears). Simply there not enough information to understand what they're purposed.

If the problme is "I whant CHelper to be also able to copy", then you should probably better to folow an idiom like this (#include and using namespace apart ...)

class CHelper
{
    struct impl
    {
       .....
    };
public:
    // create and initialize
    CHelper() :pimpl(new impl) {}

    // move: just keep the default
    CHelper(CHelper&& a) = default;

    // copy: initialize with a copy of impl
    CHelper(const CHelper& a) :pimpl(new impl(*a.pimpl)) {}

    CHelper& operator=(CHelper a) //note: pass by value and let compiler do the magics
    { 
        pimpl = move(a.pimpl); //a now nullifyed, but that's ok, it's just a value
        return *this; 
    }

    ~CHelper() = default; //not really necessary
private:
    unique_ptr<impl> pimpl;
};

Of course, feel free to separate declarations and implementation as you need.

EDIT following John Balcom comments.

Yes, of course the code changes, but not in its substance. You just can forward declare struct impl; into CHelper (so that unique_ptr has a meaning), then declare struct CHelper::impl somewhere else (may be in the CPP file where all CHelper immplementation will be done).

The only attention, here, is that in the CPP file CHelper::impl must have both constructor and destructor defined, in order to have a coherent unique_ptr instantiation (that has to call the impl destructor) inside the CPP file. Otherwise there is the risk, with some compilers, to get an "incomplete type usage" error in all files that include the CHelper declaration.

About the second point (deriving from std::set) this is a controversial aspect of C++ programming. For reason that are not pertinet to C++ itself, but with the Object Oriented Programming school, "inhertance" means "is a", and "is a" means "object subtitution possible". Because of that, since deleting an object via a base pointer is UB if the base dtor isn't virtual, thus making object soubstitution UB, the OOP school refuse as a dogma the inheritance of whatever class having no virtual dtor and because of the way they had been educated when they started to program, they start to spit their flame on you if you do that.

To me, that's not a problem in your design, but their fault in understanding that C++ inheritance does not means "is a" but "is like" and does not imply object substitution (to me it is their fault in thinking every C++ class is an OOP object, not you using a tool to do something useful to you, just look here or here, if you want more clarification on my position: object substitution in C++ is not for "object" but method by method since every method can be virtual or not independently on every other). That's said, may be you have to work with those people, so ... evaluate pros and cons in not following them in the practice of their own favorite religion.