Cppcoreguidelines: Is rule C.67 too inflexible?

[jwakely]

C.67 A base class should suppress copying, and provide a virtual clone instead if "copying" is desired

Enforcement
A class with any virtual function should not have a copy constructor or copy assignment operator (compiler-generated or handwritten).

That rule requires deleting all the copy/move ops, but is that rule too strong? Isn't it OK to make them protected, so only derived classes can copy/move the base class?

class B {
public:
  virtual ~B() = default;
  // virtual functions ...
protected:
  B(const B&) = default;
  B& operator=(const B&) = default;
  B(B&&) = default;
  B& operator=(B&&) = default;
};

class D final : public B {
public:
  ~D() = default;
  D(const D& other) : B(other) // ...
  D& operator=(const D& other) // ...
  D(D&& other) noexcept : B(other) // ...
  D(& operator=(D&& other) noexcept // ...
  // ...
};

Or even if the derived type isn't copyable and provides a D::clone() member as per C.67, the base class' copy constructor can still be useful to implement that (if a copy is made inside the D::clone() override).

Alternatively, if the base is an abstract class then it can't be sliced anyway (and also can't define a sensible clone() function so it has to be pure virtual).

Maybe the enforcement could be improved, so that it's OK to have copy/move ops if they are protected, or if the class is abstract.

[blakehawkins]

It would be nice to have a concrete counterexample to C.67 where a derived class wants to copy for a good reason.

[MikeGitb]

I certainly want to be able to copy a Circle even if it is derived from a IShape and considering that a circle is probably just 3 doubles (center & radius) I prefer to not use clone for that (which requires a dynamic memory allocation an returns either a raw pointer or a smart_pointer to the base class) if it isn't necessary.

[hsutter]

Mike, could you elaborate on that example? Starting with IShape:

~
class IShape { /...public pure virtual functions.../ };
~

Does IShape suppress copying? It's an ABC, so it ought to, right?

Then Circle:

~
class Circle : public IShape {
/* circle data /
public:
Circle(Circle const&) { /.../ }
Circle& operator=(Circle const&) { /...*/ }
};
~

How would you write those functions? What would they do?

Then finally:

~~~
class BlueCircle : public Circle {
/* more data /
public:
/ copying? */
};

BlueCircle get_circle();

int main() {
Circle c = get_circle();
}
~~~

Would BlueCircle also be copyable? Is the slicing in main intended?

[MikeGitb]

@hsutter:

What I said was meant in support of what jwakely suggested: protected copy construction/assignment should be allowed. Now if you really want to do public implementation inheritance you can solve that problem via another level of indirection:

struct IShape{
    virtual void draw() = 0;
    virtual std::unique_ptr<IShape> clone() = 0;
    virtual ~IShape() = default;
protected:
    IShape() = default;
    IShape(const IShape&) = default;
    IShape& operator=(const IShape&) = default;
};

struct CircleImpl : IShape {
    void draw() override { std::cout << "Draw Circle"; };
protected:
    CircleImpl() = default;
    CircleImpl(const CircleImpl&) = default;
    CircleImpl& operator=(const CircleImpl&) = default;
};

struct Circle final : CircleImpl {
    Circle() = default;
    Circle(const Circle&) = default;
    Circle& operator=(const Circle&) = default;

    std::unique_ptr<IShape> clone() override { return std::make_unique<Circle>(*this); }
};

struct BlueCircleImpl : CircleImpl {
    void draw() override { std::cout << "Draw BlueCircle"; }
protected:
    BlueCircleImpl() = default;
    BlueCircleImpl(const BlueCircleImpl&) = default;
    BlueCircleImpl& operator=(const BlueCircleImpl&) = default;
};

struct BlueCircle final : BlueCircleImpl {
    BlueCircle() = default;
    BlueCircle(const BlueCircle&) = default;
    BlueCircle& operator=(const BlueCircle&) = default;

    std::unique_ptr<IShape> clone() override { return std::make_unique<BlueCircle>(*this); }
};

BlueCircle get_circle() { return BlueCircle{}; }
std::unique_ptr<IShape>  circle_factory(bool colored) {
    if (colored) {
        return std::make_unique<BlueCircle>();
    } else {
        return std::make_unique<Circle>();
    }
}

int main() {
    std::unique_ptr<IShape> s = circle_factory(true);
    auto s2 = s->clone();

            auto c = get_circle();
            auto c2 = c;

    Circle c = get_circle();  // compiler error -> no accidential slicing
}

My general rule of thumb is: Don't inherit publicly from classes with public constructors or assignment operators and vice versa, mark any class with public constructors final.

Regardingthe polymorphic types, I think it is important to realize two distinct cases: In the one case, the object is created by some factory on the heap and only ever accessed through the base class interface. In that case, there is obviously no real need for copy constructors and such .

The other case however is, where the object is treated just like an normal object in one part of the program (created on the stack, copied around, accessed through expressions of the "correct" dynamic type), but is then presented through a polymorphic interface to another part

[hsutter]

Back to Jonathan's original question: Should protected copying be allowed for a base class?

We think the answer is no: Since there will already be a clone function, the derived implementation can already access that and any copying would be in addition and duplicative.

Also, as a general principle we are trying to make the guidelines restrict what is possible in the language rather than support every coding style.

Does that make sense?

[MikeGitb]

@hsutter:

How does the base classe's clone function solve that problem? The base version's clone function will create a dynamically allocated base class object. How can you turn that into a derived object after the fact? Could you demonstrate how you would do it?

Even worse: If my base class is an abstract class, it can't even implement clone in the first place.

I certainly don't have knowledge about a representative c++ code base with millions and millions of lines of code, but my feeling is that what you call "[not] support every coding style" effectively outlaws a lot of established practice (of course some people consider dynamic polymorphism the work of the devil so restricting it might be exactly what you want).

[hsutter]

Update from 4/16 editors call: @MikeGitb That's a good point. We will think more about this question.

[MikeGitb]

Any update on this?

[justanotheranonymoususer]

Stumbled upon this as well. See also:
https://stackoverflow.com/questions/46918749/how-to-clone-an-object-without-copy-constructor