2

If one wants to implement Clone pattern in C++, he might not be sure about safety, because derived class may forget to override it:

struct A {
    virtual A* Clone() const {
        return new A(*this);
    }
}

struct B : A {
    int value;
};

int main() {
   B b;
   // oops
   auto b_clone = b.Clone();
   delete b_clone;
}

What are the possible ways to improve Clone pattern in C++ in this regard?

A more general question has been asked: Forcing a derived class to overload a virtual method in a non-abstract base class

However, it seems to be too general to have a good solution in C++ -- the discussion is about possible ways to enforce method override. I'm more interested in discovering a useful pattern, which might help in the exact case of using Cloneable pattern.

Nikita Petrenko
  • 1,068
  • 1
  • 7
  • 10
  • 2
    Derived class not overriding it has nothing to do with safety. lack of virtual destructor in base class and use of naked pointers does. – user7860670 Jul 10 '19 at 20:11
  • @VTT for such trivial classes it is ok – Nikita Petrenko Jul 10 '19 at 20:12
  • 3
    No, it’s not ok but a major cause for undefined behaviour! – dtell Jul 10 '19 at 20:21
  • @datell first of all, there are no destructors in the example, even compiler-generated ones. So I don't believe you this is an UB – Nikita Petrenko Jul 10 '19 at 20:24
  • 1
    It is UB, see [when-to-use-virtual-destructors](https://stackoverflow.com/questions/461203/when-to-use-virtual-destructors). – Jarod42 Jul 10 '19 at 20:36
  • @Jarod42, well, that's interesting. Though I'm not sure if there is a compiler for which it indeed causes problems (e.g., no compiler warns about such things, as well as UBsan) – Nikita Petrenko Jul 10 '19 at 20:44
  • @NikitaPetrenko hmm, you sure about there not even being compiler generated ones? –  Jul 10 '19 at 20:45
  • If there are no destructors, then what is `delete b_clone;` calling? – user4581301 Jul 10 '19 at 20:45
  • @user4581301 what is `int* ptr; delete ptr;` calling? E.g., you cannot call `ptr->~int()`. – Nikita Petrenko Jul 10 '19 at 20:55
  • @NikitaPetrenko Thats the same question as "what is `int main() { int a; }` calling?" Of course not a dtor but thats since `int` is a pod. To make it clear: All the classes above do have dtors: The default ones. – dtell Jul 10 '19 at 21:05
  • @NikitaPetrenko I mean yes, you can never call a destructor directly (to my knowledge, anyway), but that doesn't they don't still exist. They have to exist. Deletion is impossible otherwise. –  Jul 10 '19 at 21:06
  • @Chipster Of course you can it manually call it if a user defined type is used. `int` and other PODs don't have dtors. However this is ot here. – dtell Jul 10 '19 at 21:12
  • Possible duplicate of [Forcing a derived class to overload a virtual method in a non-abstract base class](https://stackoverflow.com/questions/12632747/forcing-a-derived-class-to-overload-a-virtual-method-in-a-non-abstract-base-clas) – dtell Jul 10 '19 at 21:13
  • @Chipster Manual destruction is important to [placement new](https://en.cppreference.com/w/cpp/language/new#Placement_new). And you can delete the destructor. Can't think of a good use-case for doing this though. – user4581301 Jul 10 '19 at 21:13
  • But apparently other people could: [Uses of destructor = delete;](https://stackoverflow.com/questions/40742918/uses-of-destructor-delete) – user4581301 Jul 10 '19 at 21:15
  • @datell it is not a duplicate -- the question you linked asks about a more general problem – Nikita Petrenko Jul 10 '19 at 21:16
  • @NikitaPetrenko It actually talks about the exactly same problem and your's reduces to this one imo. – dtell Jul 10 '19 at 21:16
  • @user4581301 I mean `delete` calls the destructor, but you wouldn't really ever do something like the OP suggested, correct? I mean syntax wise. –  Jul 10 '19 at 21:18
  • @Chipster There are times when you want to delete the object without giving back the memory. The allocation could be expensive (so you use a resource pool instead) or risky (fragmentation for one thing) or impossible (on some critical systems access to the dynamic datastore is cut off after program initialization to prevent uncontrolled memory usage) and you want to reuse the memory. C++ is a Swiss army knife and there are tools in it that you're likely to never use. – user4581301 Jul 10 '19 at 21:25

2 Answers2

0

This is an elaboration of one of the answers, suggesting runtime check using typeid: Forcing a derived class to overload a virtual method in a non-abstract base class

Using CRTP, one can come up with the following basic idea:

Create class Cloneable<Derived>, which manages cloning for Derived, and adds all the needed runtime checks (seems like that compile-time checks are not possible even with CRTP).

However, it is not trivial, and one also has to manage inheritance through Cloneable, as described:

#include <memory>
#include <cassert>
#include <type_traits>
#include <typeinfo>

class CloneableInterface {
public:
    virtual std::unique_ptr<CloneableInterface> Clone() const = 0;
};

template <class... inherit_from>
struct InheritFrom : public inherit_from... {
};

template <class Derived, class AnotherBase = void, bool base_is_cloneable = std::is_base_of_v<CloneableInterface, AnotherBase>>
class Cloneable;

// three identical implementations, only the inheritance is different

// "no base is defined" case
template <class Derived>
class Cloneable<Derived, void, false> : public CloneableInterface {
public:
    std::unique_ptr<CloneableInterface> Clone() const override {
        assert(typeid(*this) == typeid(Derived));
    return std::make_unique<Derived>(static_cast<const Derived&>(*this));
    }
};

// Base is defined, and already provides CloneableInterface
template <class Derived, class AnotherBase>
class Cloneable<Derived, AnotherBase, true> : public AnotherBase {
   ...
};

// Base is defined, but has no CloneableInterface
template <class Derived, class AnotherBase>
class Cloneable<Derived, AnotherBase, false> : public AnotherBase, public CloneableInterface {
    ...
};

Usage example:

class Base : public Cloneable<Base> {
};

// Just some struct to test multiple inheritance
struct Other {
};

struct Derived : Cloneable<Derived, InheritFrom<Base, Other>> {
};

struct OtherBase {
};

struct OtherDerived : Cloneable<OtherDerived, InheritFrom<OtherBase>> {
};

int main() {
    // compiles and runs
    auto base_ptr = std::make_unique<Base>();
    auto derived_ptr = std::make_unique<Derived>();
    auto base_clone = base_ptr->Clone();
    auto derived_clone = derived_ptr->Clone();

    auto otherderived_ptr = std::make_unique<OtherDerived>();
    auto otherderived_clone = otherderived_ptr->Clone();
}

Any critics and improvement suggestions are welcome!

Nikita Petrenko
  • 1,068
  • 1
  • 7
  • 10
0

C++17 and newer offers an std::any. You could in theory, then, make a clone function that returns an std::any* instead:

struct A {
    virtual std::any* Clone() const {
        return new A(*this);
    }
}

struct B : A {
    int value;
    // I suppose it doesn't have to be virtual here, 
    // but just in case we want to inherit the cloning capability from B as well
    virtual std::any* Clone() const { // Note: you still need to override this function
        return new B(*this);          // in the lower levels, though
    }
};
// Note: I'm still on MSVS2010, so this C++17 code is untested.
// Particularly problematic could be this main
int main() {
   B b;
   // Here is the clone
   auto b_clone = std::any_cast<B*>(b.Clone());
   delete b_clone;
}

Again, this is untested, but it should work in theory.

  • Well, ok, I get your joke about delegating all the responsibility to the one who calls it, but this is not a pattern improvement in any way :) – Nikita Petrenko Jul 10 '19 at 21:14
  • Maybe I'm misunderstanding your problems with the current pattern. What are your issues with the current pattern? –  Jul 10 '19 at 21:15
  • 1
    @Chipster The problem is that you could forget to override in B. – dtell Jul 10 '19 at 21:18
  • I think I misunderstood. That actually seems very legit, though I've never worked with `std::any` – Nikita Petrenko Jul 10 '19 at 21:22
  • @datell I recon. My philosophy was that no matter if a child object overrides it or not, it can only return an `A*`, not a `B*` as you would expect. This solves that issue. I suppose there is still nothing in my solution keeping you from not overriding the function, though. Might have to think on that. –  Jul 10 '19 at 21:22
  • @Chipster though I think it is not very good, because it might be very hard to debug -- consider that you have a `class Graph` of clonable `Nodes`, and you wish to copy it, but some user forgot to override -- he'll get a trash instead of a good graph, and the bug might be hard to localize. My solution has no such problem – Nikita Petrenko Jul 10 '19 at 21:30
  • @Chipster I wanted to say that the bug may be not even evident -- provided that one never calls `std::any_cast` – Nikita Petrenko Jul 10 '19 at 22:02
  • *"it can only return an `A*`, not a `B*` as you would expect. This solves that issue."* You think so? The entire point of `Clone` is that caller doesn't know the actual (dynamic) type of the object it's called on (otherwise you could just use a copy constructor). Since you don't know the correct type, how are you going to call `std::any_cast`? – HolyBlackCat Jul 10 '19 at 22:02