Base class pointer as private member

Question

I am working on a system where a class has a private member that is a base class pointer. The pointer can be pointing to an inherited class object. I would like the parameterized and copy constructors to be able to take a pointer of the base class type and deep copy it without erasing the inherited type. Here is some code I made to demonstrate this problem; I would like c2 and c3 to call B.print instead of A.print, but can't figure out how.

#include <iostream>

class A
{
protected:
    double a;
    double b;

public:
    A()
    {
        a = 0.0;
        b = 0.0;
    }

    A(double a, double b)
    {
        this->a = a;
        this->b = b;
    }

    A(const A& copy)
    {
        a = copy.a;
        b = copy.b;
    }

    virtual void print()
    {
        std::cout << "print A" << std::endl;
        std::cout << "a: " << a << ", b: " << b << std::endl;
    }
};

class B : public A
{
private:
    double c;

public:
    B()
    {
        c = 0.0;
    }

    B(double a, double b, double c) : A(a, b)
    {
        this->c = c;
    }

    B(const B& copy) : A(copy)
    {
        c = copy.c;
    }

    virtual void print()
    {
        std::cout << "print B" << std::endl;
        std::cout << "a: " << a << ", b: " << b << ", c: " << c << std::endl;
    }
};

class C
{
private:
    A* a;

public:
    C()
    {
        a = nullptr;
    }

    C(A* a)
    {
        this->a = new A(*a);
    }

    C(const C& copy)
    {
        a = nullptr;
        if (copy.a != nullptr)
            a = new A(*copy.a);
    }

    ~C()
    {
        delete a;
        a = nullptr;
    }

    void print()
    {
        if (a != nullptr)
            a->print();
    }
};

int main()
{
    A* a = new A(3.5, 4.5);
    A* b = new B(3.5, 4.5, 5.5);
    C c1(a), c2(b), c3(c2);
    delete a, b;

    c1.print();
    c2.print();
    c3.print();
}

I have tried including an enum of the available types and storing that value separately, but that program was non-extensible as it would require changing a switch and enum for each added inherited type.

this creates an `A` not something else `this->a = new A(*a);` — 463035818_is_not_an_ai, Feb 07 '23 at 18:23
90% of this is a red herring. The problem is this line `this->a = new A(*a);`. It cannot work this way. You want to read up about making a `clone` virtual method. ` — n. m. could be an AI, Feb 07 '23 at 18:26
ot: `a = nullptr;` in the destructor is pointless. Imho it must be removed. I would not let it pass a code review. — 463035818_is_not_an_ai, Feb 07 '23 at 18:29
[How to clone a derived object in C++](https://stackoverflow.com/questions/24939570/how-to-clone-as-derived-object-in-c) — BoP, Feb 07 '23 at 18:32
`delete a, b;` doesn't do what you think it does: It's ***not*** equivalent to `delete a; delete b;` but to `delete a; b;` (or even simpler just `delete a;`), since `delete` is an unary operator with a higher precedence than the `,` operator. Btw: I recommend making use of `std::unique_ptr` instead of using `new` + `delete` manually... — fabian, Feb 07 '23 at 19:12

Useless · Accepted Answer · 2023-02-07T22:51:58.040

Your actual problem is with these lines:

    C(A* a) {
        this->a = new A(*a);
    }

    C(const C& copy) {
        a = nullptr;
        if (copy.a != nullptr)
            a = new A(*copy.a);
    }

which explicitly create an object with dynamic type A, ignoring the dynamic types of a and copy.a.

The usual pattern for copying something's real dynamic type is

struct Base
{
    // copy ctor etc. as usual
    virtual Base* clone() const { return new Base(*this); }
};

struct Derived: Base
{
    virtual Base* clone() const { return new Derived(*this); }
};

Other, miscellaneous notes:

You don't need to burn 6 lines on a default constructor that does nothing any more. Just write
```
class A
{
protected:
    double a{};
    double b{};
```
and let the compiler default it. You can default most of your copy constructors too.
If you're controlling pointer lifetime (as C is), just use std::unique_ptr. It's there, it's free, it's much more explicit about your semantics.
When you're owning (ie, controlling the lifetime of) a polymorphic object via a base-class pointer, that base class needs to have a virtual destructor (unless you're doing something exotic with a type-erased deleter, which is out of scope here).
If you've decided to use this->a for data members, use it consistently. If you're only using it for disambiguation, just change the parameter/local variable name so it doesn't collide in the first place.

There are member-naming conventions like m_a or just a_ which make it easy to see which variables are data members, remove ambiguity, and are still less typing/visual noise than randomly sprinkling this-> around.

Working code with only the changes described above:

#include <iostream>
#include <memory>

class A
{
protected:
    double a_{};
    double b_{};

public:
    A() = default;
    A(double a, double b) : a_(a), b_(b) {}
    A(const A&) = default;
    virtual ~A() = default;
    virtual A* clone() const { return new A(*this); }

    virtual void print()
    {
        std::cout << "print A" << '\n'
                  << "a: " << a_ << ", b: " << b_ << '\n';
    }
};

class B : public A
{
    double c_{};

public:
    B() = default;
    B(double a, double b, double c) : A(a, b), c_(c) {}
    B(const B&) = default;

    virtual B* clone() const override { return new B(*this); }

    virtual void print() override
    {
        std::cout << "print B" << '\n'
                  << "a: " << a_ << ", b: " << b_ << ", c: " << c_ << '\n';
    }
};

// owns and deep-copies an A.
// class invariant: ptr_ is non-null
//
class C
{
private:
    std::unique_ptr<A> ptr_;

public:
    explicit C(A* a) : ptr_(a->clone()) {}
    C(const C& copy) : ptr_(copy.ptr_->clone()) {}

    void print()
    {
        ptr_->print();
    }
};

int main()
{
    A a{3.5, 4.5};
    B b{3.5, 4.5, 5.5};
    C c1(&a), c2(&b), c3(c2);

    c1.print();
    c2.print();
    c3.print();
}

NB. The top-level dynamic allocation was unnecessary even if you do want C to deep copy, so I just removed it entirely instead of either changing it to also use unique_ptr or fixing that incorrect delete statement.

Also classes with `virtual` methods should virtually alwyas have a `virtual` destructor — Mooing Duck, Feb 07 '23 at 22:41
I knew starting to fix the obvious problems would open a can of worms. Good point! — Useless, Feb 07 '23 at 22:43

Base class pointer as private member

1 Answers1