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:

[CODE=cpp]class Solution
{
private:
int *HeuristicTours ;

public:
int* GetTours()
{
return HeuristicTours;
}
};[/CODE]

[CODE=cpp]int run(Problem &p)
{
int **m_ppiTourMatr ix_bestsofar; // declared globally so cleanup() can access
int *piNext, *piTours;
...
piNext = p.solution.GetT ours();
for (i=0; i<p.solution.He uristicVehicleC ount; 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_ppiTourMatri x_bestsofar != NULL)
{
free(m_ppiTourM atrix_bestsofar );
m_ppiTourMatrix _bestsofar = NULL;
}
[/CODE]

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 [CODE=cpp]*piNext = m_ppiTourMatrix _bestsofar[i][j];[/CODE] 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::GetTo urs() 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.He uristicVehicleC ount; 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_ppiTourMatr ix_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_ppdPheromon eMatrix_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:

[CODE=cpp]
double **m_ppdPheromon eMatrix_vei;

// elsewhere

void function()
{
double **ppdPheromoneM atrix;
ppdPheromoneMat rix = m_ppdPheromoneM atrix_vei;

// and is used like so:

ppdPheromoneMat rix[iLastNode][iNextNode] += dTmp2;
}[/CODE]

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

[CODE=cpp]// declared like below in a function

std::vector<std ::vector<double >> PheromoneMatrix (iSize, std::vector<dou ble>(iSize, 0));

// then passed to another function

void function(std::v ector<std::vect or<double>> &PheromoneMatri x)
{
// Then replace all ppdPheromoneMat rix references with PheromoneMatrix
PheromoneMatrix[iLastNode][iNextNode] += dTmp2;
}[/CODE]

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_ppdPheromon eMatrix_vei;

// elsewhere

void function()
{
double **ppdPheromoneM atrix;
ppdPheromoneMat rix = m_ppdPheromoneM atrix_vei;

// and is used like so:

ppdPheromoneMat rix[iLastNode][iNextNode] += dTmp2;
}

OK, let's assume m_ppdPheromoneM atrix_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_ppdPheromoneM atrix_vei to identify it. You don't need ppdPheromoneMat rix becuse you never change the address inside ppdPheromoneMat rix. Your code should work without it.

[code=cpp]
double **m_ppdPheromon eMatrix_vei;

// elsewhere

void function()
{

m_ppdPheromoneM atrix_vei[iLastNode][iNextNode] += dTmp2;
}
[/quote]

When you replace thjis with a vector<vector<d ouble> > you have a vector of vectors and not a 2D array that you can fiddle with indiscriminatel y. That is, the equivalent of
[code=cpp]
m_ppdPheromoneM atrix_vei[iLastNode][iNextNode] += dTmp2;
[/code]

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.