Values disappearing from array on cleanup

[IanWright]

I've got a section of a program that I can't quite get to work. I'm fairly sure its something very simple/trivial but it looks correct to me, so if someone could help me fix the problem, and explain what it is that is wrong, that would be great... I've posted a sample of code, which is the bit of interest:

class Solution
{
private:
   int *HeuristicTours;
 
public:
    int* GetTours()
    {
        return HeuristicTours;
    }
};

int run(Problem &p)
{
   int **m_ppiTourMatrix_bestsofar; // declared globally so cleanup() can access
   int *piNext, *piTours;
...
   piNext = p.solution.GetTours();
   for (i=0; i<p.solution.HeuristicVehicleCount; i++)
   {
      iCount = m_ppiTourMatrix_bestsofar[i][0];
 
      for (j=1; j<=iCount; j++)
      {
         *piNext = m_ppiTourMatrix_bestsofar[i][j];
         piNext++;
      }
 
      *piNext = -1;
      piNext++;
   }
   cleanup();
   return 0;
}
 
void cleanup()
if (m_ppiTourMatrix_bestsofar != NULL)
{
   free(m_ppiTourMatrix_bestsofar);
   m_ppiTourMatrix_bestsofar = NULL;
}
 

m_ppiTourMatrix_bestsofar is populated with values to start with, so the aim is to copy these values to the p.solution tours. This then can iterate through the tours later, using -1 as a termination of a tour.

The problem I have is that although the values are set fine before calling cleanup(), as soon as cleanup() has been called my values within my solution tours are now uninitalised.

I thought that

*piNext = m_ppiTourMatrix_bestsofar[i][j];

would copy the value within the matrix, into the value stored at location piNext, but if that were the case then freeing up m_ppiTourMatrix_bestsofar shouldn't make any difference should it?

Thanks for your help.

[weaknessforcats]

Several things aren't correct here.

1) Solution::GetTours() returns a pointer to a Solution private data member. That breaks encapsulation and makes the private data member dangerous to use since it can be altered(even deleted) by nefarious agents who do not belong to the class.

I wouldn't do that. Unstead, write methods to return and update a specific element in this array.

2) This -1 business is really another private Solution data member foe the number of elements. The Solution member funcitons use this to control themseleves.

3) This code:
[code=cpp]
for (i=0; i<p.solution.HeuristicVehicleCount; i++)
[/quote]

infers that there is a public data member. You don't need that. What you need is a method to position to the start of your HeuristicTours array and another to tell you when you have reached the end. That is, this is C++ so why are you not using a vector<int>?

4) Global variables are a big no-no. Read this article http://bytes.com/forum/thread737451.html. In this case m_ppiTourMatrix_bestsofar shoulkd be a function argument.

5) m_ppiTourMatrix_bestsofar is an int** and this should be
used as the address of a 2D array. Read this article: http://bytes.com/forum/thread772412.html.

6) m_ppiTourMatrix_bestsofar is an int**. This is the address of an int pointer.
Freeing this does not free the stuff the int pointer points at. It just frees the pointer to the pointer. If the address in the pointer m_ppiTourMatrix_bestsofar points at is on the heap, you leak. Again why are you not using a vector here?

7) After you call cleanup(), the global variable is zero. Next time you use it, you crash.

8) What dos this mean:

run(Problem &p)
{
int **m_ppiTourMatrix_bestsofar; // declared globally so cleanup() can access
etc...

How can you say this? m_ppiTourMatrix_bestsofar is a local int** inside the
run() funciton. If there is a global variable, this local one hides it.


Post again when these concerns are addressed.

[IanWright]

weaknessforcats,

Thanks for the response. I understand that there are a number of problems, I'm trying to integrate some existing code with my own to test as a prototype, once I can prove that it works then the next step is to tidy it all up...

At present I am trying to figure something out, I can't really post the code because its quite complex and scattered all over. As an example though, I'm trying to change a double **m_ppdPheromoneMatrix_vei to a vector (similar to the suggestion you made about the int** variable).

Hopefully once I've managed to change a few of these simpler ones, I can start changing the others which are a bit more widely used within the program. I'm having a problem however in doing this and would appreciate any suggestions:

Currently:

double **m_ppdPheromoneMatrix_vei;
 
// elsewhere
 
void function()
{
double **ppdPheromoneMatrix;
ppdPheromoneMatrix = m_ppdPheromoneMatrix_vei;
 
// and is used like so:
 
ppdPheromoneMatrix[iLastNode][iNextNode] += dTmp2;
}

I am trying to change, as per your suggestion to pass around a vector:

// declared like below in a function
 
std::vector<std::vector<double>> PheromoneMatrix(iSize, std::vector<double>(iSize, 0));
 
// then passed to another function
 
void function(std::vector<std::vector<double>> &PheromoneMatrix)
{
   // Then replace all ppdPheromoneMatrix references with PheromoneMatrix
  PheromoneMatrix[iLastNode][iNextNode] += dTmp2;
}

This however causes my code to no longer work, and I am trying to figure out whether there is a fundamental difference in the way these are treated/used as I would have expected the program to behave exactly the same... instead of accessing an array to instead access the vector items.

[weaknessforcats]

double **m_ppdPheromoneMatrix_vei;

// elsewhere

void function()
{
double **ppdPheromoneMatrix;
ppdPheromoneMatrix = m_ppdPheromoneMatrix_vei;

// and is used like so:

ppdPheromoneMatrix[iLastNode][iNextNode] += dTmp2;
}

OK, let's assume m_ppdPheromoneMatrix_vei is the address of a 2D matrix of double. Inside the function() you wish to change the array so you can just use m_ppdPheromoneMatrix_vei to identify it. You don't need ppdPheromoneMatrix becuse you never change the address inside ppdPheromoneMatrix. Your code should work without it.

[code=cpp]
double **m_ppdPheromoneMatrix_vei;

// elsewhere

void function()
{

m_ppdPheromoneMatrix_vei[iLastNode][iNextNode] += dTmp2;
}
[/quote]

When you replace thjis with a vector<vector<double> > you have a vector of vectors and not a 2D array that you can fiddle with indiscriminately. That is, the equivalent of

m_ppdPheromoneMatrix_vei[iLastNode][iNextNode] += dTmp2;
 

won't work unless the element is already in the vector. You can't add an element with a value of dTmp2 to a vector this way.

Your vector sample is not clear that the vectors already constain the elements and you are just changing the values of already existing elements.

[IanWright]

Thanks weaknessforcats, I believe that I've understood it correctly, so I'm just going to take a deeper look to try and find out what's going wrong. The vectors are populated upon their creation so that shouldn't be a problem.