Problems with functions in a class with pointer to self

[BigDaddyDrew]

Hi guys,

I've got a class that has a member variable which is a pointer to an object of the same class. I'm having problems dealing with some of the constructors (mainly copy) and assignment operator. I don't think I've ever written classes with this special case, although I realize it shouldn't be too hard. If you have any ideas on how to best rewrite my copy construct, assignment operator, or anything else (trivial constructors??), please let me know!

Thanks, Andrew

Location.h

#ifndef LOCATION_H
#define LOCATION_H

using namespace std;

class Location
{
private:
    int row, col, cost;
    Location* history;

public:
    Location()
    { row = col = cost = 0; history = NULL; }
    Location(int r, int c)
    { row = r; col = c; cost = 0; history = NULL; }
    Location(const Location& loc);
    ~Location()
    { delete history; history = NULL; }
    int getRow() const
    { return row; }
    int getCol() const
    { return col; }
    int getCost() const
    { return cost; }
    void setCost(int c)
    { cost = c; }
    void setHistory(Location loc)
    { *history = loc; }
    Location& getHistory() const
    { return *history; }
    bool operator < (const Location& loc) const
    { return (cost > loc.getCost()); }
    Location& operator =(const Location& loc);   
};

#endif



Location.cpp

#include"location.h"

using namespace std;

Location::Location(const Location& loc)
{
    if (loc.history != NULL)
    {
        row = loc.row; col = loc.col; cost = loc.cost;
    }
    history = new Location();
    *history = loc.getHistory();
}

Location& Location::operator =(const Location& loc)
{
    if (this != &loc)
    { 
        row = loc.row; col = loc.col; cost = loc.cost;
        if (history != NULL) delete history;
        history = new Location(loc.getHistory()); 
    }
  
    return *this;
}

[master5001]

You aren't dereferencing your pointers correctly. More correctly, I should say you are dereferencing something that you shouldn't.

*history = loc.getHistory();

Is incorrect. The best way to do this would be

history = loc.history;

Even though history is private you can do this since they are both the same class. You could also take the address of getHistory()'s return and set history equal to that, but my example is much cleaner.

[BigDaddyDrew]

I understand what you're doing. I think initially I was thinking that we can't do history = loc.history, because that would just be assigning the pointer, rather than the history element, but, I'm pretty sure I want to assign them to point at the same object.

I'll see how that works,
Thanks

[Sebastiani]

It appears that you are creating an endless loop, too. Look:

Location& Location :: operator =(const Location& loc)
{
 //...
        history = new Location(loc.getHistory()); 
 //...
}

This will in turn operator = ad infinitum via the new Location(Location&) call!

One solution is to simply duplicate operator = 's code in the constructor in order to prevent the infinite recursion:

(Also rewrote getHistory() to prevent raising an exception if history == NULL)

Location& getHistory() const
{
  if(history == 0) history = new Location(); 
  return *history; 
}


Location::Location(const Location& loc)
{
    
  if (loc.history != NULL)
    {
        getHistory().row = loc.getHistory().row; 
        getHistory().col   = loc.getHistory().col; 
        getHistory().cost = loc.getHistory().cost;
    }
    
       row = loc.row; 
       col = loc.col; 
       cost = loc.cost;
 }

[BigDaddyDrew]

Sorry for the absence,

I had elected to go another route, but am decided again to go with this method, of a pointer to a Location.

My question regarding the presented code is this:

Will the assignment operator and copy constructor adequetly copy the history. In other words: I'm going to need to get at the history of the history of the history, etc. So, I can't just copy the element, I need to get history to point at the actualy history element.

Perhaps that makes more sense?

Thanks!

[grib]

May I suggest creating a location object that knows nothing of it's history and another object that maintains a list/vector of locations. Have the constructor create a location and push_back that location to your history, then have all your methods that would not have created a new location route thier calls to history.back().member_fn(); The methods that would have added a new state to your history push this new state to the back.

If your history is constant grab boost and use a shared_ptr, this will allow shallow copies (new locations little more expensive than copying a pointer) but destruction is handled cleanly (there can be no cycles)

[BigDaddyDrew]

Thanks, I'll give some of that a go.