How to rewrite code not to call virtual function from the constructor

Question

So shortly the situation is like this

class Base
{
public:
    Base() {  setZero();}
    virtual void setZero() {std::cout << "Set all Base class values to zeros (default) values";}
};

class Derived : public Base
{
public:
    Derived () { }
    
    void setZero() override {
        Base::setZero(); 
        std::cout << "Set all Derived class values to zeros (default) values";
    }
};

setZero is public an is called form different places, also it has some logic, not just assignments, as Base and Derived classes are quite large. But it's all doesn't work as intended as dynamic binding doesn't work when function is called from the constructor. I see the solution to duplicate code from setZero to the consructors, but duplication of code is a bad thing. Is there some other solutions?

Answer 1

If I understand your question correctly, then what you need to do is below simply

#include <iostream>
using std::cout;
using std::endl;

class Base
{
    void init() {std::cout << "Set all Base class values to zeros (default) values" << endl;}
public:
    Base() {init(); }
    virtual void setZero() {init();}
};
class Derived : public Base
{
    void init() { std::cout << "Set all Derived class values to zeros (default) values" << endl; }
public:
    Derived () { init(); }

    void setZero() override {
        Base::setZero();
        init();

    }
};
int main()
{
    Derived d1;
    cout << endl;
    d1.setZero();
}

You wrote the below statement for your code

But it's all doesn't work as intended as dynamic binding doesn't work when function is called from the constructor.

Yes, the virtual behavior will not work, when calling setZero() from the base class constructor, and the reason is that derived class has not been constructed yet.

What you need is to initialize each class when its constructed, and that should happen in there respective constructors, and that is what we do in the above code.

Base class constructor will call its own setZero, derived class constructor will call its own setZero.

And you will continue to do the same thing, if you derive any further class from Derived class.

Answer 2

You could implement a simple factory method in your Derived class and remove the setZero() calls from the constructors alltogether. Then making the constructors non-public will tell consumers of the class to use the factory method for proper instantiation instead of the constructor. Something like this:

class Base
{
protected:
    Base() { }
    virtual void setZero() {std::cout << "Set all Base class values to zeros (default) values";}
};

class Derived : public Base
{
public:
    static Derived createInstance()
    {
        Derived derived;
        derived.setZero();
        return derived;
    }    
private:
    Derived() { }
    
    void setZero() override {
        Base::setZero(); 
        std::cout << "Set all Derived class values to zeros (default) values";
    }
};

And then create your instance of Derived somehow like this:

int main()
{
    Derived derived = Derived::createInstance(); 
    // do something...
    
    return 0;
}

With this approach you can also make sure that no one can create an instance of your class that is not in a valid state.

Note: Don't know if you use the base class at some places directly but if this is the case you could provide a factory method for it as well.

Answer 3

Alternatively to the other answers, separating functionality from API lets you use the general flow you want while dodging the whole "using the vtable in the constructor" issue.

class Base
{
public:
    Base() {
      setZeroImpl_();
    }

    virtual void setZero() { 
      setZeroImpl_(); 
    }

private:
  void setZeroImpl_() {
    std::cout << "Set all Base class values to zeros (default) values";
  }
};

class Derived : public Base
{
public:
    Derived () {
      setZeroImpl_();
    }
    
    void setZero() override {
        Base::setZero(); 
        setZeroImpl_();
    }

private:
  void setZeroImpl_() {
    std::cout << "Set all Derived class values to zeros (default) values";
  }
};

Answer 4

TL;DR - two phase construction sucks. Try to make your constructors construct stuff, and not call any virtual methods, or require it in order to function.

---

If you want initialization to occur after object construction (including vtables), you need to have a separate initialization phase on your objects.

A probably better way to handle this is this:

class Base
{
  int x = 0; // notice the =0 here
public:
  Base() {} // nothing
  virtual setZero() {*this = Base{};} // use operator= to assign zeros
};
class Derived : public Base
{
  double d = 0.; // notice the = 0. here
public:
  Derived () { } // nothing

  void setZero() override {*this = Derived{};}
};

we can avoid rewriting setZero as well:

template<class D, class B=void>
struct SetZero:B {
  void setZero() override {
    *static_cast<D*>(this) = D{};
  }
};
template<class D>
struct SetZero<D,void> {
  virtual void setZero() {
    *static_cast<D*>(this) = D{};
  }
};

now we can:

class Base:public SetZero<Base>
{
  int x = 0; // notice the =0 here
public:
  A() {} // nothing
};
class Derived : public SetZero<Derived, Base>
{
  double d = 0.; // notice the = 0. here
public:
  Derived () { } // nothing
};

and setZero is written for us.

The DRY here is that default construction zeros, and we put the zeros right next to where we declare variables. setZero then just becomes a helper method to copy over yourself with a default constructed object.

Now, exposing value semantics copy/move operations on a class with a vtable is a bad plan. So you probably want to make the copy/move protected and add friend declarations.

template<class D, class B=void>
struct SetZero:B {
  void setZero() override {
    *static_cast<D*>(this) = D{};
  }
  SetZero()=default;
protected:
  SetZero(SetZero&&)=default;
  SetZero& operator=(SetZero&&)=default;
  SetZero(SetZero const&)=default;
  SetZero& operator=(SetZero const&)=default;
  ~SetZero() override=default;
};

template<class D>
struct SetZero<D,void> {
  virtual void setZero() {
    *static_cast<D*>(this) = D{};
  }
  SetZero()=default;
protected:
  SetZero(SetZero&&)=default;
  SetZero& operator=(SetZero&&)=default;
  SetZero(SetZero const&)=default;
  SetZero& operator=(SetZero const&)=default;
  virtual ~SetZero()=default;
};

so those get longer.

In Base and Derived as they have vtables, you are recommended to add

protected:
  Derived(Derived&&)=default;
  Derived& operator=(Derived&&)=default;
};

to block external access to move/copy construct and move/copy assign. This is advised regardless of how you write setZero (any such move/copy is going to risk slicing, so exposing it to all users of your class is a bad plan. Here I make it protected, because setZero relies on it to make zeroing DRY.)

---

Another approach is a two-phase construction. In it, we mark all "raw" constructors are protected.

class Base {
  int x;
protected:
  Base() {} // nothing
public:
  virtual setZero() { x = 0; }
};

we then add a non-constructor constructor:

class Base {
  int x;
protected:
  Base() {} // nothing
public:
  template<class...Ts>
  static Base Construct(Ts&&...ts){
    Base b{std::forward<Ts>(ts)...};
    b.setZero();
  }
  virtual setZero() { x = 0; }
};

and external users have to Base::Construct to get a Base object. This sort of sucks, because our type is no longer regular, but we already have vtable, which makes it unlikely to be regular in the first place.

We can CRTP it;

template<class D, class B=void>
struct TwoPhaseConstruct:B {
  template<class...Ts>
  D Construct(Ts&&...ts) {
    D d{std::forward<Ts>(ts...));
    d.setZero();
    return d;
  }
};
template<class D>
struct TwoPhaseConstruct<D,void> {
  template<class...Ts>
  D Construct(Ts&&...ts) {
    D d{std::forward<Ts>(ts...));
    d.setZero();
    return d;
  }
};

class Base:public TwoPhaseConstruct<Base> {
  int x;
protected:
  Base() {} // nothing
public:
  virtual setZero() { x = 0; }
};
class Derived:public TwoPhaseConstruct<Derived, Base> {
  int y;
protected:
  Derived() {} // nothing
public:
  virtual setZero() { Base::setZero(); y = 0; }
};

and here goes down the rabbit hole, if you want to make_shared or similar we have to add a helper type.

template<class F>
struct constructor_t {
  F f;
  template<std::constructible_from<std::invoke_result_t<F const&>> T>
  operator T()const&{ f(); }
  template<std::constructible_from<std::invoke_result_t<F&&>> T>
  operator T()&&{ std::move(f)(); }
};

which lets us

auto pBase = std::make_shared<Base>( constructor_t{[]{ return Base::Construct(); }} );

but how far down the rabbit hole do you want to go?

Answer 5

You could solve it this way:

#include <iostream>

class Base
{
public:
    Base() {  Base::setZero();}
    virtual void setZero() {std::cout << "Set all Base class values to zeros (default) values\n";}

protected:
    Base(bool) {};
};

class Derived : public Base
{
public:
    Derived () : Base(true) { Derived::setZero(); }
    
    void setZero() override {
        Base::setZero(); 
        std::cout << "Set all Derived class values to zeros (default) values\n";
    }
};

What I have done, is the following:

1. Make clear which setZero() method is called by which constructor
2. Added call to setZero()also from the Derived constructor
3. Added a protected Base constructor that does not call its setZero() method, and calling this constructor from Derived's constructor, so that Base::setZero()is called exactly once during creation of a Derived object.

By doing it this way, you can create Base or Derived and call zerZero() as intended.

Answer 6

You might have factory to have "post-call", something like:

template <typename T, typename ... Ts>
T CreateBaseType(Ts&&... args)
{
    T t(std::forward<Ts>(args)...);
    t.setZero();
    return t;
}