2

I'm having some trouble working out how to write a nicely polymorphic annealer class. I'm sure I'm doing something very wrong, and that this is a duplicate question but I have been searching for a long time and not found anything.

I want to take a pointer to an instance of the abstract type State (called pState) and use the pure virtual functions Perturb() (Which randomly changes the state a bit) and Cost() (Which evaluates the cost of the function) to find a state that minimises the cost function.

I need to keep track of multiple subclass instances in this function, and I believe that either the declaration or assignment to these variables is causing an issue.

void Annealer::Minimise(State *pState){
    //Set up 
    ....
    ....
    State *state;
    State *newState;

    *state = *pState;
    *newState = *state;

    //Evaluate the initial cost
    pState->Cost(); //Works just fine
    double cost = state->Cost(); //Segfault

    ....
}

Calling Cost on the subclass pointer before the attempt to copy is successful. I have not included the subclass because it is very long and complex.

Edit:

State is defined as follows:

class State {
public:
    virtual void Perturb()=0;
    virtual double Cost()=0;
};
Theo Emms
  • 23
  • 4
  • You should post source code of State class. I would assume the bug might be related to copying objects and not pointers in: – Anorflame Mar 07 '16 at 19:25
  • 2
    Between the definition of the `state` variable and the `*state = *pState` assignment, do you make `state` actually point somewhere valid? Same question with `newState`. Have you thought about making those two variables (`state` and `newState`) non-pointer variables? – Some programmer dude Mar 07 '16 at 19:25
  • How would I make them point somewhere valid? I want to avoid using dynamic memory allocation if possible. – Theo Emms Mar 07 '16 at 19:29
  • Creating dynamic memory is necessary depending on what you are trying to do... are you trying to make changes to the original object or make a copy of the object before making changes. – LINEMAN78 Mar 07 '16 at 19:31
  • I posted this as an answer, but notice that only the State part is copied in the assignment. Then you attempt to call a virtual function, which location is calculated by adding an offset to the starting addresss of the object, according to the virtual table. But since the only the 'State' part of the actual object is copied, calling the virtual function at the resulting address would cause a segfault. – Anorflame Mar 07 '16 at 19:36
  • @Anorflame How would I resolve this? – Theo Emms Mar 07 '16 at 19:43
  • I would suggest adding clone() function, as someone already suggested. If you don't have access to the State class, then MAYBE, you could try figuring the actual class using dynamic_cast for example. (returns null if conversion fails). And then call the right copy constructor. – Anorflame Mar 07 '16 at 19:45
  • Okay so the solution is to add a pure virtual clone function that passes back a `new Subclass(...)`? To answer @LINEMAN78 I want to copy the object itself to several locations in memory. I want to avoid functions that return pointers that must be deleted though. – Theo Emms Mar 07 '16 at 19:51
  • That's right, thats the solution. You can use smart pointers to avoid having to call delete. Basically, smart pointer is an object that may be allocated statically, it holds the actual pointer as a member and calls 'delete pointer' in it's destructor. – Anorflame Mar 07 '16 at 19:55
  • Thank you all fot the help! – Theo Emms Mar 07 '16 at 20:03

3 Answers3

2

The problem is here:

*state = *pState;
*newState = *state;

You are attempting to copy pState into wherever the state variable is pointing, which has not been set. You need to allocate memory if you are going to copy it:

virtual State* State::clone() = 0;
State *state;
State *newState;
state = pState->clone();
newState = state->clone();
// do some stuff
delete state;
delete newState;

or keep it as a pointer:

State *state;
State *newState;
state = pState;
newState = state;

Or a hybrid of the 2 options, in the case of simply making a copy to make changes and copying to the main variable if valid and no errors:

virtual State* State::clone() = 0;
virtual void State::copy(State* cpy) = 0;
State *state;
State *newState;
state = pState;
newState = state->clone();
... // do stuff to newState
state->copy(newState); // copy changes to input
delete newState;

Edit: Due to issues with polymorphism and copy constructors, you can implement a pure virtual clone method, but this will cause non-stack allocations and you will need to delete or use an auto pointer.

Copy object - keep polymorphism

Community
  • 1
  • 1
LINEMAN78
  • 2,562
  • 16
  • 19
  • I don't think I can do the first option as State contains pure virtual functions. And I think the second option would cause state and newState to point to the same place, whereas I want two **different** pointers to values that are copies of `*pState`. – Theo Emms Mar 07 '16 at 19:33
  • Agreed, consider this: http://stackoverflow.com/questions/22008053/polymorphism-with-copy-constructor However, this method is no longer a stack allocation, so you will need to remember to delete the local variables. – LINEMAN78 Mar 07 '16 at 19:36
0

Yeah, you declare the state variable as a pointer, then dereference the pointer that has an unknown value, so does not point to a valid object. That is a bad idea and will ultimately result in a segfault or access to an undefined memory region. If you don't know the proper state subclass both when calling the method and inside of it, I would say you should have, for example, a pure virtual clone member function that would create the copy of the current state. Of course it would look almost the same in subclasses. But, somehow I believe it may be somewhat ugly, not sure what do others think, I am not an experienced c++ dev.
If your call site, that is the function passing the state pointer, knows the proper subclass, then you could consider using templates, even combined with polymorphism.

Michał Zegan
  • 456
  • 4
  • 12
  • Ahh, if the state is an abstract class, then you can only copy pointers, usually. – Michał Zegan Mar 07 '16 at 19:32
  • Is there a way I can obtain a copy of `*pState` on the stack to point to? – Theo Emms Mar 07 '16 at 19:35
  • But I need `state` and `newState` to be able to point to different values, so I don't think copying pointers will allow me to achieve what I want. – Theo Emms Mar 07 '16 at 19:37
  • I am wondering if it would be a good idea to have something like a clone() virtual method that would allocate a new instance as a copy. The problem here is that you need a second instance to copy data to, but you don't know the subclass of the abstract class that you would need to create. – Michał Zegan Mar 07 '16 at 19:39
  • Yes, the problem really is that I don't know the type of the object and so can't call the right assignment/copy/whatever operator/constructor. – Theo Emms Mar 07 '16 at 19:42
  • Answer edited (most of it). Please comment as I am afraid I said something wrong. – Michał Zegan Mar 07 '16 at 19:52
  • Hello, I think you mentioned clone() first so I awarded you the answer. I think it's the most viable solution. It's just unfortunate that other users of the code will have to delete the pointer. – Theo Emms Mar 07 '16 at 20:01
  • To clarify, I was (probably) the only one suggesting a virtual clone method. About deletion, you can use things like smart pointers if your compiler supports c++11 – Michał Zegan Mar 07 '16 at 20:03
0

Also, notice that the copy constructor that is called, is the one of class State. It may cause a problem if Cost's implementation depends somehow on the actual class's data also to be copied in the assignment.

Anorflame
  • 376
  • 3
  • 12