Instantiating a new object. Not sure if this is correct?

Question

I have a member pointer with this type:

const TopState<TestHSM>* state_;

state_ is of polymorphic type.

TopState is the base:

template<typename H>
struct TopState {
//... functions etc
};

There is a bit of a hierarchy and finally LeafState which is not abstract:

template<typename H, unsigned id,
typename B=CompState<H,0,TopState<H> > >
struct LeafState : B {
   //...functions
   static const LeafState obj;
};

The following objects represent state:

//indentation to indicate state nesting
typedef CompState<TestHSM,0>      Top;
typedef CompState<TestHSM,1,Top>    S0;
typedef CompState<TestHSM,2,S0>       S1;
typedef LeafState<TestHSM,3,S1>         S11;
typedef CompState<TestHSM,4,S0>       S2;
typedef CompState<TestHSM,5,S2>         S21;
typedef LeafState<TestHSM,6,S21>          S211;

Note only S11 and S211 (LeafState's) can be instantiated.

I have a TestHSM class which looks like this:

class TestHSM {
public:
TestHSM()  {
    state_ = new S11;
}

//fix destruction - problem
~TestHSM() {
    //reset to s11
//  state_ = &S11;
//  delete state_;
//  state_ = 0;
}

void next(const TopState<TestHSM>& state)
{ 
    state_ = &state; 
}

private:
const TopState<TestHSM>* state_;
};

My problem right now is creation of the state_ object. See constructor above. This works but I am not entirely sure if it is correct way to do things?

S11 is the first state where the object can be instantiated - and in my program S11 is the first state at startup. The code 'works' as expected. Or seems to anyway. But I am not sure that this is the optimal or even correct way to instatiate the fist state?

In addition, if I do this then I get heap memory runtime errors when I attempt to delete state_ - see commented out destructor code.

Any suggestions?

Angus

Can you make the code minimal? – Luchian Grigore Mar 24 '12 at 09:44 — Luchian Grigore, Mar 24 '12 at 09:44

score 0 · Accepted Answer · answered Mar 24 '12 at 09:52

Your object construction and usage is correct and standard C++. The only problem is with your destruction code as you note. You are trying to take a pointer of a type - this is not going to work and the compiler won't let you do that. The destructor should be as simple as freeing the memory:

~TestHSM() {
  if (state_)  {
    delete state_;
    state_ = 0;
  }
}

Also, be sure when you change states to delete the previous state. I.e. the next() function should look like this:

void next(TopState<TestHSM> *state)
{ 
    if (state_)
      delete state_;
    state_ = state; 
}

You should always pass a state constructed using new to the next function and let TestHSM free it when it's unnecessary:

int main()  {
  TestHSM test;
  test.next(new S211());
  // No freeing, TestHSM destructor frees everything
}

score 0 · Answer 2 · edited May 23 '17 at 12:20

That class TestHSM presented is not following the rule of three. It's missing a copy constructor and copy assignment operator, or a way to forbid them.

Your best option is to let someone else handle the resource management for you. For example, use a std::unique_ptr, or boost::scoped_ptr member. Either of these options will forbid copies of TestHSM. If the ability to copy is desirable, you will have to write a copy constructor by hand, along with some virtual copy mechanism (i.e. clone() member functions) for the states.

Instantiating a new object. Not sure if this is correct?

2 Answers2