Take a look at this code

int* piTemp=new int[ChangeNumber];
		
for (int x=0;x&ltChangeNumberx++)
{
    piTemp[x]=GetDocument()-&gtNumsfiItems[x];
}

if (GetDocument()-&gtNumsfiItems)
{
    delete[]GetDocument()-&gtNumsfiItems//A
    GetDocument()-&gtNumsfiItems=NULL;
}
		
GetDocument()-&gtNumsfiItems=
                    new int[GetDocument()-&gtNumTimesChanged];
		
for (x=0;x&ltChangeNumberx++)
{
    GetDocument()-&gtNumsfiItems[x]=piTemp[x];
}

if (piTemp)
{
    delete[]piTemp;//B
    piTemp=NULL;
}

 

At point A everything is fine I delete GetDocument()NumsfiItems and piTemp still holds valid numbers. However at point B when i delete piTemp it also changes the value in GetDocument()NumsfiItems (to a random int). Can anyone see where Im going wrong? Is it because NumsfiItems is a class member whereas piTemp is a local variable? Because it seems that that is the only difference I can find. I am doing the exact same thing twice, first time round (A) deleting the ptr does nothing to the ptr i want to keep but second time round (B) deleting the ptr also deletes the ptr i want to keep. If anyone can help I would be most grateful because Im at a loss. Thanks

What makes you think it is deleting the other pointer? If you run in debug mode, the compiler should fill deleted blocks with a certain value (I think it is 0xDDDDDDDD) so you shouldnt get random numbers, but consistent ones.

I do have one question - are you sure that changeNumber is > 0?

The code looks correct to me, if somewhat messy. But it should work. As Sandman said though, you are using some numbers that we can''t verify. You should consider stepping through it and ensuring that all these numbers are valid. (For example, ChangeNumber needs to be less than or equal to NumTimesChanged, and so on.)

Shouldn''t your second new allocate ChangeNumber elements, or assign ChangeNumber into NumTimesChanged before allocation? NumTimesChanged seems uninitialized, so you''re probably writing past the end of allocation.

Thanks for the help guys. After Sandman and Kylotan suggested that ChangeNumber might be dodgy I stepped through it again but carried on past the part i posted and found that i was doing GetDocument()-&gtNumsfiItems=new int[ChangeNumber] which I didnt see before so that is why it was altered.

Hey Kylotan out of interest how could I do the same thing without it being messy? Because I have pieces of code very similar to this littered around my app.

Thanks again

Ok. I''m guessing this bit of uncommented code (there: that''s the first ''problem'' with it: we have to guess its intent) is to allow you to change the size of the NumsfiItems array (whatever NumsfiItems might mean). You copy everything across to a temporary array, free the old one, allocate a new one of a different size, and copy the stuff back. Yes? (If no, then it''s still not my fault: you should have commented it properly )

So, simple answer #1: use std::vector, since you''re using C++. If NumsfiItems is a vector instead of an array, that code becomes this:

GetDocument()->NumsfiItems.resize(GetDocument()->NumTimesChanged); 

Yes, the whole lot condenses into just one line. No pointer juggling or explicit memory allocation or handling of temporaries. It copies the values over for you. Of course, you would need to learn a little about std::vectors to use them properly, but it''s either that or keep reinventing the wheel. And if the prospect of reducing 12 lines of code into 1 line of code all across your app doesn''t make you want to learn the STL, perhaps nothing will.

Ok, more minor points:

Don''t bother setting pointers to NULL when the very next line is just going to reallocate them again. It confuses the purpose of your code. Only set pointer to NULL when the rest of the program considers NULL to be a valid value for that pointer. Similarly, checking "if (piTemp)" is a bad idea. piTemp should always be valid: you just allocated it yourself. If piTemp happens not to be valid, you want it to crash first time: because this shows that there is an error in your code somewhere. If you''re going to test piTemp for NULL-ness, that should be done after it''s allocated and before anything dereferences it. Because checking at the end does you no favours.

Lastly, this is pointless:

if (GetDocument()->NumsfiItems){    delete[]GetDocument()->NumsfiItems; 

It is perfectly valid to call delete on a NULL pointer. It was designed that way. So just call delete without checking what it was. And as I said above, don''t set it to NULL, because you''re reassigning in the very next line anyway.

Thanks Kylotan. Yeah you are correct about the intent of the code it is to resize the NumsfiItems Array. Ill have a look at stl especially the std::vector because there are so many instances in my code where i have to resize an array and to do just one line instead of many will make things so much easier (especially when i dont have to keep calling new and delete).
Hmm the only reason i always set a ptr to NULL when i delete it is because andre lamothe said in TOTWGPG that to be safe do so, but I can see what you are saying- there is no pointing in setting it to NULL if straight away im setting it to something else.

I try to prevent my app from crashing but the idea of not checking piTemp so that it does crash makes sense because then I know that something is wrong.

I didnt realise you could call delete on a null pointer I thought that gave an unhandled exception error so that is why I always check before i delete anything but if its possible then ill just let it delete it.

Thanks a lot Kylotan, I really appreciate the tips. (and in future Ill remember to comment any code i post here )

quote:

---

Original post by Zeke
Hmm the only reason i always set a ptr to NULL when i delete it is because andre lamothe said in TOTWGPG that to be safe do so, but I can see what you are saying- there is no pointing in setting it to NULL if straight away im setting it to something else.

I try to prevent my app from crashing but the idea of not checking piTemp so that it does crash makes sense because then I know that something is wrong.

---

Yeah. I understand that I disagree with the Mighty LaMothe on this issue. Personally, I would rather have a program that crashes so that I can fix the problem. Other people would rather have a program that will keep running regardless of problems: so they don''t have to fix them. The question you have to ask yourself is: do you want to be a ''good'' programmer, or just a ''good-enough'' programmer?

Basically, if there should always be a value in a pointer, never set it to NULL, because you want a NULL state to be obvious. But if NULL is valid, then set it to NULL when necessary. While you are developing, crashes are your friend, because they show you a defect in your code early on. If you write code that edges around defects and ignores them, you might not discover that defect until a later stage, at which point it could take forever to fix.