How not to use "new"

[slotdev]

I know that in SFML-world, using "new" is very bad practise.

I have a std::map vector of sprites/sounds/etc, held in the public section of a class, and I reference those objects by a string.

This means that, when I create the objects, I have to "new" them. Is there any other way to do this.

I'm getting heap corruption when trying to delete the objects, and memory leaks if they're not deleted. I suspect this is why I shouldn't be using new  :oops:

[Laurent]

I know that in SFML-world, using "new" is very bad practise.

Nop. It's more "in the C++ world, handling deallocations (and most of the allocations) manually is a very bad practice".

I have a std::map vector of sprites/sounds/etc, held in the public section of a class, and I reference those objects by a string.

This means that, when I create the objects, I have to "new" them.

Why? You can have default-constructed instances and assign/modify them later.

I'm getting heap corruption when trying to delete the objects, and memory leaks if they're not deleted. I suspect this is why I shouldn't be using new

Absolutely

[slotdev]

Right, that's all working. All "new" with SFML stuff is gone. Happy days. Well almost.

My sprites and other data/functions I need are contained in a "SpriteObj" class, and I create an instance of this for each sprite. Fine.

When I call the destructor, it breaks with a heap pointer error. Here's the call stack trace. Any ideas?? I know I shouldn't use it but I don't know how to do this any other way!

    ntdll.dll!77ea0844()    
    [Frames below may be incorrect and/or missing, no symbols loaded for ntdll.dll]   
    ntdll.dll!77e62a74()    
    ntdll.dll!77e1cd87()    
    KernelBase.dll!7617468e()    
    PR1027.exe!_CrtIsValidHeapPointer(const void * pUserData=0x04335f00)  Line 2072   C++
    PR1027.exe!_free_dbg_nolock(void * pUserData=0x04335f00, int nBlockUse=1)  Line 1279 + 0x9 bytes   C++
    PR1027.exe!_free_dbg(void * pUserData=0x04335f00, int nBlockUse=1)  Line 1220 + 0xd bytes   C++
    PR1027.exe!operator delete(void * pUserData=0x04335f00)  Line 54 + 0x10 bytes   C++
    PR1027.exe!std::allocator<std::_Tree_nod<std::_Tset_traits<sf::ResourcePtr<sf::Image> *,std::less<sf::ResourcePtr<sf::Image> *>,std::allocator<sf::ResourcePtr<sf::Image> *>,0> >::_Node>::deallocate(std::_Tree_nod<std::_Tset_traits<sf::ResourcePtr<sf::Image> *,std::less<sf::ResourcePtr<sf::Image> *>,std::allocator<sf::ResourcePtr<sf::Image> *>,0> >::_Node * _Ptr=0x04335f00, unsigned int __formal=1)  Line 141 + 0x9 bytes   C++
    PR1027.exe!std::_Tree<std::_Tset_traits<sf::ResourcePtr<sf::Image> *,std::less<sf::ResourcePtr<sf::Image> *>,std::allocator<sf::ResourcePtr<sf::Image> *>,0> >::erase(std::_Tree<std::_Tset_traits<sf::ResourcePtr<sf::Image> *,std::less<sf::ResourcePtr<sf::Image> *>,std::allocator<sf::ResourcePtr<sf::Image> *>,0> >::iterator _Where=0x0426c7e4 {myResource=0x04267dd4 })  Line 910   C++
    PR1027.exe!std::_Tree<std::_Tset_traits<sf::ResourcePtr<sf::Image> *,std::less<sf::ResourcePtr<sf::Image> *>,std::allocator<sf::ResourcePtr<sf::Image> *>,0> >::erase(std::_Tree<std::_Tset_traits<sf::ResourcePtr<sf::Image> *,std::less<sf::ResourcePtr<sf::Image> *>,std::allocator<sf::ResourcePtr<sf::Image> *>,0> >::iterator _First=0x0426c7e4 {myResource=0x04267dd4 }, std::_Tree<std::_Tset_traits<sf::ResourcePtr<sf::Image> *,std::less<sf::ResourcePtr<sf::Image> *>,std::allocator<sf::ResourcePtr<sf::Image> *>,0> >::iterator _Last=0x0426c7e4 {myResource=0x04267dd4 })  Line 926 + 0x3f bytes   C++
    PR1027.exe!std::_Tree<std::_Tset_traits<sf::ResourcePtr<sf::Image> *,std::less<sf::ResourcePtr<sf::Image> *>,std::allocator<sf::ResourcePtr<sf::Image> *>,0> >::erase(sf::ResourcePtr<sf::Image> * const & _Keyval=0x0426c244)  Line 936 + 0x67 bytes   C++
>   PR1027.exe!sf::Resource<sf::Image>::Disconnect(sf::ResourcePtr<sf::Image> & Observer={...})  Line 88   C++
    PR1027.exe!sf::ResourcePtr<sf::Image>::~ResourcePtr<sf::Image>()  Line 69   C++
PR1027.exe!sf::Sprite::~Sprite()  + 0x5b bytes   C++
PR1027.exe!SpriteObj::~SpriteObj()  Line 35 + 0x21 bytes   C++

[Laurent]

When I call the destructor

Do you mean that you do it explicitely? You mustn't. Destructors are always called automatically when the variable reaches the end of its scope.

[slotdev]

No sorry, I use "delete". I get memory leaks detected for each SpriteObj instance created if I don't use "delete". But I get heap corruption if I do.

[Laurent]

If you don't use new to allocate them, you must not use delete to destroy them. If they are members of a class, they are automatically destroyed when the owner instance is destroyed. So maybe it's this one that is not properly destroyed?

You should definitely show us some code.

[slotdev]

Here's some code. I have a feeling it's being caused by the destructor in sf::Image being called twice, but that's just a hunch:

Class that holds my std::map of my "SpriteObj" objects.

Code: [Select]


class GfxEngine
{
public:
    GfxEngine(void);
    ~GfxEngine();
    std::map<string,SpriteObj*>gameSprites;
    void createSprites();
};


Stuff that actually instatiates the SpriteObj's - don't worry about the xmlData stuff, I've cut that out for readability, and it all works.

Code: [Select]


void GfxEngine::createSprites()
{
// Get some data out of an XML file and create a SpriteObj
gameSprites[xmlData.gfxId] = new SpriteObj(&xmlData);
}


The SpriteObj class:

Code: [Select]


class SpriteObj
{
public:
SpriteObj(struct GfxResources::_GfxDataStruct_ * gfxData);
~SpriteObj();
// Data
Sprite spritePtr; // THE ACTUAL sf:Sprite (not a pointer to!)
private:
struct GfxResources::_GfxDataStruct_ spriteData;
};


SpriteObj's constructor & destructor:

Code: [Select]


// SpriteObj constructor  
SpriteObj::SpriteObj(struct GfxResources::_GfxDataStruct_ * gfxData) : spritePtr(Sprite())
{
// Create this sprite from the data taken from the XML array
spritePtr.SetImage(gfxEngine->gfxTextures[gfxData->gfxFile]);
spritePtr.SetSubRect(sprRect);
spritePtr.SetPosition((float)gfxData->gfxDx,(float)gfxData->gfxDy);
}
// SpriteObj destructor
SpriteObj::~SpriteObj()
{
// Goes BANG here - presumably calling sf::Image destructor
}


Finally, in the destructor of GfxRngine, where I delete all instances of the SpriteObj objects, excuse the use of vectors copying the name of the sprite but I am debugging

Code: [Select]


// GfxEngine destructor
GfxEngine::~GfxEngine()
{
std::vector<string> gsStrings;
std::map<string,SpriteObj*>::iterator it;
for (it = gameSprites.begin(); it != gameSprites.end(); ++it)
{
gsStrings.push_back(gameSprites[it->first]->GetName());
}
std::vector<string>::const_iterator i;
for (size_t i = 0; i < gsStrings.size(); i++)
{
// This does call the destructor of each gameSprite
delete gameSprites[gsStrings[i]];
}
}


Thanks
Ed

[Laurent]

Do you really need pointers to SpriteObj in your map? What is this GfxResources::_GfxDataStruct_ structure? It looks like it's not used (at least initialized) at all in SpriteObj.

By the way, gameSprites[it->first] is the same as it->second

[slotdev]

Do you really need pointers to SpriteObj in your map? What is this GfxResources::_GfxDataStruct_ structure? It looks like it's not used (at least initialized) at all in SpriteObj.

By the way, gameSprites[it->first] is the same as it->second

Hmm, I could probably remove the pointers altogether. Would that actually solve the problem? I don't know. If the destructor to sf::Image is being called twice, then no.

[Laurent]

If the destructor to sf::Image is being called twice, then no.

Where is there an sf::Image instance? Why would it be destroyed twice? Can you please answer all my questions (what is GfxResources::_GfxDataStruct_ ?)?

[Disch]

This isn't your problem but this function can be greatly improved:

Blech:

Code: [Select]


// GfxEngine destructor
GfxEngine::~GfxEngine()
{
   std::vector<string> gsStrings;
   std::map<string,SpriteObj*>::iterator it;
   for (it = gameSprites.begin(); it != gameSprites.end(); ++it)
   {
      gsStrings.push_back(gameSprites[it->first]->GetName());
   }
   std::vector<string>::const_iterator i;
   for (size_t i = 0; i < gsStrings.size(); i++)
   {
// This does call the destructor of each gameSprite
      delete gameSprites[gsStrings[i]];
   }
}


Better:

Code: [Select]


// GfxEngine destructor
GfxEngine::~GfxEngine()
{
   std::map<string,SpriteObj*>::iterator it;
   for (it = gameSprites.begin(); it != gameSprites.end(); ++it)
   {
      delete it->second;
   }
   gameSprites.clear();   // although even this line is superfluous, since
      // it will be cleared automatically by its dtor.
}



EDIT:

actually maybe this is your problem.  If GetName isn't returning the exact same thing as it->first, you'll be attempting to delete non-existing items in your map, which would cause it to explode.

[slotdev]

Laurent:

GfxResources::_GfxDataStruct_  is just a structure I fill from reading in data from XML (sprite name, w/h src x/y dst x/y and so on) which I will then use to create the sprite. I didn't put that in the code I posted here as it's not relevant.

The sf::Image stuff is as in that stack trace above - I assume sf::Sprite uses sf::Image somewhere along the way?

I think I do need to get rid of these pointers. They're nothing but trouble

Disch:

Thanks for the modification, I have implemented it but it didn't fix the problem. I read somewhere that you shouldn't delete stuff from vectors while iterating over it, as the number of elements in the vector might not match and it all goes bang. Not sure.

[Disch]

I think I do need to get rid of these pointers. They're nothing but trouble

You need them for polymorphism  (if you're using polymorphism).  Otherwise, yeah you probably shouldn't be using them here.

Although this problem is likely more due to heap corruption than anything else.  Are you stepping outside of an array bounds somewhere?  Or dereferencing a bad pointer?

I find it very unlikely that you stumbled upon a SFML bug, as such a fundamental bug in the sf::Sprite->sf::Image relationship probably would have been exposed and corrected already.  Heap corruption is seeming more and more likely.

Is it possible for you to upload the entire program somewhere so we can take a look?

I read somewhere that you shouldn't delete stuff from vectors while iterating over it

It probably was saying you shouldn't erase stuff from vectors while iterating over it (as in, actually removing the elements from the vector).

There are reasons not to do that, although you can if you do it properly.  Note however that's not what I'm doing here.  I'm keeping all the elements in the map, just deleting the pointers.

[slotdev]

I have commented out all references to the sf::Sprite in my class and all references to it, recompiled & run, and the deleting of the SpriteObj class instances all works fine, no mem leaks/heap corruption etc.

Now, I am slowly adding things back in, just the creation of the spritePtr in the SpriteObj constructor firstly (runs/exits OK), but then adding the following call in the constructor, the crash/heap corruption occurs (when eventually deleting the instance of SpriteObj).

Code: [Select]


spritePtr.SetImage(gfxEngine->gfxTextures[gfxData->gfxFile]);


gfxTextures is an array of type sf::Image, all of which are valid. The program runs fine and displays the right graphics etc, so that's fine.

Soooo....that brings me back to this sf::Image destructor or something being at fault. I can't believe it's SFML, this would almost certainly have been found by now as Disch said.

[Laurent]

Can you show us more code related to the gfxEngine->gfxTextures array?

When is your SpriteObj destroyed? Is it at global exit?