Sign in to follow this  
Wavesonics

C++ Tips on debugging memory corruption

Recommended Posts

I have a very bad bug that I'm almost certain is stemming from memory corruption. I run a whole bunch of delete and delete [] and then some time very shortly afterwards the whole program crashes at a random point. Thats the tell tale signs of memory corruption as far as I know. So I was wondering if any one had any good tips on trying to locate or detect these types of bugs. Anything at all, I've tried a lot of things but maybe theres some basic technique people use that I don't know so please, anything no matter how simple tell me how you guys usually track these bugs.

Share this post


Link to post
Share on other sites
I (and many other people who know what they're doing) generally sidestep this by using the "basic technique" of RAII to avoid creating the problems in the first place. Show the code, and I'll try to show how these things can be fixed.

Share this post


Link to post
Share on other sites
Quote:
Original post by Wavesonics
I have a very bad bug that I'm almost certain is stemming from memory corruption.

I run a whole bunch of delete and delete [] and then some time very shortly afterwards the whole program crashes at a random point. Thats the tell tale signs of memory corruption as far as I know.

So I was wondering if any one had any good tips on trying to locate or detect these types of bugs. Anything at all, I've tried a lot of things but maybe theres some basic technique people use that I don't know so please, anything no matter how simple tell me how you guys usually track these bugs.


It really depends on what system you are using to develop your code on. If it is Visual Studio then it has some pretty good CRT debug memory tools that you can enable - they will slow your code down a lot but it can do heap checks to make sure that any allocation/deallocation hasn't blown the heap and it also puts guard bands around allocations to make sure that you haven't walked off the end of an array and so on.

On top of that I also use a memory manager by Paul Nettle:

http://www.flipcode.org/cgi-bin/fcarticles.cgi?show=64444

to track all allocations and deallocations - its pretty good at what it does.

And finally, for the extreme bugs (i.e. those that are really hard to find) I start using PCLint... but I have to be desperate to get to this point since running PCLint on your code is like having the harshest critic look at your code and find nothing good about it.

Share this post


Link to post
Share on other sites
Awesome, ya I had to go through a bit to find that memory manager, but I finially got it.

I've never used MSVC++ but it seems like there is quite a bit of code that is specific to the MS compiler.

But uh... wow i sound like a n00b. guess i am... How do you use it?

Share this post


Link to post
Share on other sites
So I had an idea, I decided to override the global new and delete operators to keep a count, then at certain times print out the total:


// ===============================================
// I put this in a header which all files include:
// ===============================================
#ifndef MEMREPORT
#define MEMREPORT

#include <new>
#include <cstdlib>

extern int32_t *g_iNew;
extern int32_t *g_iDelete;

extern void* operator new ( size_t size );

extern void operator delete (void *p);

extern void memReport();

#endif

// =================================
// Then I define them in my main.cpp
// in the global scope
// =================================

int32_t *g_iNew;
int32_t *g_iDelete;

void* operator new ( size_t size ) {
++(*g_iNew);

void *p = malloc( size );
return p;
}

void operator delete (void *p) {
++(*g_iDelete);
free( p );
}


// And finial I init them in main
int main(int argc, char *argv[]) {
g_iNew = (int32_t*)malloc( sizeof(int32_t) );
(*g_iNew) = 0;
g_iDelete = (int32_t*)malloc( sizeof(int32_t) );
(*g_iDelete) = 0;

...

}




But the reports are producing what seem to be none sense results.

They are in the hundreds of thousands, and TOTALLY inconsistent, every run they are different. I print the memory report before a delete and immediately after and the number has changed by many hundreds and even new has changed. So clearly my code is ef-ed.

Can any of you see a problem with it?

[edit]
These are what the results are right now when they should at the most be around 100 News:

Heap Memory Report:
Total New: 23235
Total Del: 1094
[/edit]

Share this post


Link to post
Share on other sites
If your trying to keep track of allocations and deallocations, you should also define new[] and delete[]. Why did you make g_iNew and g_iDelete pointers, anyway? A simple "int g_iNew = 0;" should work better (and they don't need to be known outside main.cpp - what's the code for memReport()?). If your trying to cut memory corruption, a good way to see if its happening is at the end of main(), g_iNew should equal g_iDelete. If it does not, than they are news that did not recieve a delete, or vice-versa.

And another thing: freeing already-freed data tends not to work.
Smart pointers, if you haven't heard of them, tend to work very well in situations like these.

Make sure every new() has a delete().

Is the program trying to access freed-data? Or null or dangling pointers?

I wonder if initializing them in main is causing havoc - or if they are being used before main() activates.

Share this post


Link to post
Share on other sites
Ya idk, i had them declared like that originally, but when it wasn't working i tried pointers wondering if it might affect the linkage.

I reverted it back now, but still get the same none sense inconsistent results.

But your very correct I need to over ride new [] and delete [] as well. Still though, not even my new and delete over rides are working how I would think they should at the moment.

Share this post


Link to post
Share on other sites
Memory breakpoints are useful for tracking down memory corruption problems. In VC++ you create a data breakpoint and have it fire if the value at a memory address that appears to be getting corrupted changes. You don't say what debugger you're using but I imagine most debuggers on the PC should have a similar facility.

Share this post


Link to post
Share on other sites
I've had quite a few circumstances where bugs like this rear their ugly heads (through maintaining someone else's code).

While the above posters are correct that there are a plethora of tools available for helping to detect these errors, nothing is fool-proof. I've had a lot of experience trying to get a tool to work and it just ends up costing me more time.

The best thing to do (especially since you don't seem to be using VS) is to comment out all but one section of code. Verify that the problem doesn't exist anymore (if it does then you've narrowed it down to the stuff that isn't commented), then uncomment your code one piece at a time until the bug shows up. You can usually use this technique to narrow the culprit down to one allocation or deallocation (or a copy somewhere).

Hope this helps...

Share this post


Link to post
Share on other sites
thanks for all your posts.

Ya i've never had a bug like this before in my 5 years of programing. I tend to code well enough this sort of thing just doesn't crop up.

But the system is at a point of interwoven complexity that it is difficult to comment out whole sections and still remain testable.

My current install of GCC for some god only knows reason didn't come with GDB so i need to get that and try some break points. I'm going to start to take some more drastic measures in creating a "clean" environment to test each little piece in. It will be time consuming but it is seeming like the only option at this point.

Share this post


Link to post
Share on other sites
Ok, here is the code used to destroy a currently loaded level. This is where the trouble starts.

void BSPManagerService::Destroy()
{
LOG->writeMsg( Logger::LVL_DEBUG, "Deleting BSP" );

m_bLoaded = false;
display( false );

// If we still have valid memory for our nodes, free them
if(nodes) {
delete [] nodes;
nodes = NULL;
numNodes = 0;
LOG->writeMsg( Logger::LVL_DEBUG, "Nodes deleted" );
}

// If we still have valid memory for our leaves, free them
if(leaves) {
string strLeafId = EMPTY_STRING;
for( int16_t i = 0; i < numLeaves; ++i ) {
strLeafId = BSP_LEAF_NODE_ID_BASE;
stringstream strBuf;
strBuf << i;
strLeafId += strBuf.str();
Leaf *ptrLeaf = dynamic_cast<Leaf*>( findElementById( strLeafId ) );
if( ptrLeaf != NULL )
delete ptrLeaf;
else
LOG->writeMsg( Logger::LVL_ERROR, "Tried to delete leaf but could not find: " + strLeafId );
}
numLeaves = 0;

delete [] leaves;
leaves = NULL;
delete [] visList;
visList = NULL;
LOG->writeMsg( Logger::LVL_DEBUG, "Leaves deleted" );
}


// If we still have valid memory for our markSurfaces, free them
if(markSurfaces) {
delete [] markSurfaces;
markSurfaces = NULL;
numMarkSurfaces = 0;
LOG->writeMsg( Logger::LVL_DEBUG, "Mark Surfaces deleted" );
}

// If we still have valid memory for our faces, free them
if(faces) {
delete [] faces;
faces = NULL;
numFaces = 0;
LOG->writeMsg( Logger::LVL_DEBUG, "Faces deleted" );
}

// If we still have valid memory for our surfedges, free them
if(surfedges) {
delete [] surfedges;
surfedges = NULL;
numSurfedges = 0;
LOG->writeMsg( Logger::LVL_DEBUG, "Surface Edges deleted" );
}

// If we still have valid memory for our edges, free them
if(edges) {
delete [] edges;
edges = NULL;
numEdges = 0;
LOG->writeMsg( Logger::LVL_DEBUG, "Edges deleted" );
}

// If we still have valid memory for our vertices, free them
if(vertices) {
delete [] vertices;
vertices = NULL;
numVerts = 0;
LOG->writeMsg( Logger::LVL_DEBUG, "Verticies deleted" );
}

if(planes) {
delete [] planes;
planes = NULL;
numPlanes = 0;
LOG->writeMsg( Logger::LVL_DEBUG, "Planes deleted" );
}

numEntities = 0;

if(textureHeader) {
// Delete the texture data from the texture manager
for( uint32_t i = 0; i < textureHeader->numMipTextures; ++i )
m_ptrTextureManagerService->deleteTexture( aryiTextureIds[i], getElementId() );

delete textureHeader;
delete [] mipTextures;
delete [] mipTextureOffsets;
delete [] faceTextureCoordinates;
delete [] textureInfo;
delete [] aryiTextureIds;
delete [] textureObjects;
LOG->writeMsg( Logger::LVL_DEBUG, "Textures deleted" );
}

LOG->writeMsg( Logger::LVL_DEBUG, "BSP Delete OK, about to delete entities..." );

for( uint32_t i = 0; i < m_entityFactories.size(); ++i )
m_entityFactories.at(i)->destroyAllEntities();

delete m_ptrTreeRoot;

LOG->writeMsg( Logger::LVL_DEBUG, "Entities deleted!" );
}






Now there are 3 entities that are deleted which are part of the class hierarchy.

Leaf is our first one:
Element -> Entity (essentially an interface) -> Resource -> Resource 3D -> Leaf

The entity factor's delete method:

void EntityFactory::destroyAllEntities() {
// Delete all memory
for( uint32_t i=0; i < m_entities.size(); ++i ) {
LOG->writeMsg( Logger::LVL_DEBUG, "About to delete entity: " + m_entities.at(i)->getElementId() );
delete m_entities.at(i);
}
// Then clear the vector of the pointers
m_entities.clear();
}






Which is used to delete the User entity:
Element -> Entity -> Resource -> Resource 3D -> User

and the camera entity:
Element -> Entity -> Resource -> Resource 3D -> Camera

Lastly the BSP Root is deleted:
Element -> Entity -> Resource -> BSPRoot

here are the destructors, they are trivial mostly except for Resource and Element, but I'll post them all:


User::~User() {
delete m_ptrTagText;
delete m_ptrFpsText;
delete m_ptrVelocityText;
delete m_ptrLeafText;
delete m_ptrPositionText;
delete m_ptrFrictionText;
delete m_ptrHudTag;
}

Leaf::~Leaf() {
}

Resource3D::~Resource3D() {
}

Resource::~Resource() {
LOG->writeMsg( Logger::LVL_DEBUG, "Deleting Resource" );
m_ptrResourceManagerService->removeResource( getElementId() );
}

Entity::~Entity() {
}

Element::~Element() {
LOG->writeMsg( Logger::LVL_DEBUG, "Deleting Element: " + getElementId() );
if( m_ptrElementManagerService->removeElementById( getElementId() ) )
LOG->writeMsg( Logger::LVL_DEBUG, "Element successfully removed from Element Manager Service: " + getElementId() );
else
LOG->writeMsg( Logger::LVL_ERROR, "Element FAILED to remove from Element Manager Service" );
}






So the last 2 important parts of the removeResource() and removeElementById() functions,

This is the removeElementById() function and the hash table remove function it calls:

bool ElementManagerService::removeElementById( string strElementId ) {
bool success = false;
LOG->writeMsg( Logger::LVL_DEBUG, "Element Manager removing element: " + strElementId );
Element *ptrElement = searchForElementById( strElementId );
// Find the elements entry in the ID hash table
ElementRegistryEntry *entry = m_ptrElementsByIDTable->retrieve( strElementId );
if( entry != NULL ) {
ClassRegistryEntry *ptrClassEntry = m_ptrElementsByClassTable->retrieve( ptrElement->getClassName() );

// Check if this is the last element
if( ptrClassEntry->m_elements.size() > 0 ) {
bool bFound = false;
for( uint32_t i=0; i < ptrClassEntry->m_elements.size() && !bFound; ++i ) {
if( ptrClassEntry->m_elements.at(i) == strElementId ) {
ptrClassEntry->m_elements.erase( ptrClassEntry->m_elements.begin() + i );
bFound = true;
}
}

// If the class is empty, remove it
if( ptrClassEntry->m_elements.size() == 0 )
m_ptrElementsByClassTable->removeByKey( ptrClassEntry->m_strClassName );
}
else
LOG->writeMsg( Logger::LVL_ERROR, "Element Manager Service: Element Class entry was empty before delete was preformed" );

// Delete the elements registry in the ID table
m_ptrElementsByIDTable->removeByKey( strElementId );
success = true;
}

return success;
}






And the hash table remove function:

// Return: -true- if key was found and chain node was removed -false- if key was not found, nothing removed
template <typename TYPE> bool HashTable<TYPE>::removeByKey( string strKey ) {
bool success = false;

uint32_t iHashValue;
iHashValue = hashString( strKey );
HashTableChainNode<TYPE> *currentNode = &table[iHashValue];

// Advance through childeren untill key match is found
while( currentNode->getChild() != NULL && !success ) {
currentNode = currentNode->getChild();

// Key match found
if( currentNode->getKey() == strKey ) {
// If parent exists, set parent's child to this nodes child
if( currentNode->getParent() != NULL )
currentNode->getParent()->setChild( currentNode->getChild() );

delete currentNode;
--m_iCurrentSize;

success = true;
}
}

return success;
}






Here is the removeResource() function:

bool ResourceManagerService::removeResource( string strElementId ) {
ResourceRegistryEntry *ptrResourceEntry = m_ptrResources->retrieve( strElementId );
// Remove scene graph roots
if( ptrResourceEntry->m_iResourceType == RMS_SG_3DROOT_NODE ) {
removeSceneGraphRoot( strElementId );
}

delete ptrResourceEntry->m_ptrSceneGraphNode;
m_ptrResources->removeByKey( strElementId );
delete ptrResourceEntry;
}

// This is conditionally called by removeResource()
bool ResourceManagerService::removeSceneGraphRoot( string strElementId ) {
bool bSuccess = false;
// Ensure Id isn't blank
if( strElementId != "" ) {
// Find the Resource Registry Entry
ResourceRegistryEntry *ptrResource = m_ptrResources->retrieve( strElementId );
// Ensure the resource was found
if( ptrResource != NULL ) {

for( uint32_t i=0; i < m_sceneGraphRoots.size() && !bSuccess; ++i ) {
// If this is the node we are looking for
if( m_sceneGraphRoots.at(i)->getNodeId() == ptrResource->m_ptrSceneGraphNode->getNodeId() ) {
m_sceneGraphRoots.erase( m_sceneGraphRoots.begin() + i );
bSuccess = true;
}
}
}
}

return bSuccess;
}






Now, the Scene Graph nodes that are deleted here call their own destructors:

// The resource node in the scene graph has a translation node and rotation node, so they must be cleaned up:
Resource3DNode::~Resource3DNode() {
delete m_ptrTranslationNode;
delete m_ptrRotationNode;
}

// Then the base destructor for SceneGraphNode is called:
SceneGraphNode::~SceneGraphNode() {
removeAllChilderen();
if( m_ptrParent != NULL ) {
m_ptrParent->removeChild( this );
}
}






And Scene Graph Nodes are:
Element -> Entity -> SceneGraphNode

So uh... ya... There's a lot going on when the resources are deleted.

Anyone see any glaringly obvious mistakes?

Share this post


Link to post
Share on other sites
There are WAY too many raw pointers, WAY too much manual memory management in that code. Why are all those members pointers? The cold fact is that humans suck at memory management, and that's why sensible people delegate that job to the computer whenever possible. In well-written C++, there are very very few instances where bare pointers are required or even desirable. RAII is your friend.

(BTW, do you know about the Rule of Three? Obeying it is absolutely necessary whenever you fiddle with bare pointers.)

Share this post


Link to post
Share on other sites
My only formal language training as been in Java and a bit of C.

Everything else is self taught which obviously has it's down sides.

I don't have any explicit copy or assignment constructors, but I pass everything around as a pointer, you want that exact instance of a thing, not the values of it, so while I see the point of the rule of 3, i don't think it has had a large affect here by not being implemented. Even still I probably will at some point implement it. Along with const correctness and other things I'm only learning about now in C++, theres quite a few things I plan on changing about this code base. Like passing references instead of pointers.

So thanks for pointing me to this info, it's all stuff I really want to learn to become a better C++ coder. Any other tips are very welcome.

Those 2 links are good thanks.

Now down to the issue though, There are certain things I can see cutting back on as raw pointers. In User, all of those members could be stack allocated, and the Rot and Trans nodes could be stack allocated. ( There is no default constructor, so I was getting compile errors?)

But besides that things need to be dynamically sized and allocated and such, so how could this be improved to remove more bare pointers?

At the moment though I don't have time to go back and refactor the entire code base to use references and less raw pointers. I need to fix this bug and finish what I'm trying to get done first, so I'm going to boot into linux nad give: http://valgrind.org/ a try. Any other sugestions?

Share this post


Link to post
Share on other sites
I see alot of this in your code but you do not show the constructors.

void BSPManagerService::Destroy()
{
LOG->writeMsg( Logger::LVL_DEBUG, "Deleting BSP" );

m_bLoaded = false;
display( false );

// If we still have valid memory for our nodes, free them
[b]if(nodes)[/b] {
delete [] nodes;
nodes = NULL;
numNodes = 0;
LOG->writeMsg( Logger::LVL_DEBUG, "Nodes deleted" );
}




Deleting a null pointer is valid in C++, so the question maybe why are you checking these values and what are the values when you check. ie do you initialise them to null in the constructor and is there any reason why the destructor can not do the destroy functions work?

Share this post


Link to post
Share on other sites
Quote:

So uh... ya... There's a lot going on when the resources are deleted.

Anyone see any glaringly obvious mistakes?


Yes: the most glaringly obvious mistake is that there is a lot going on. Well-written code is organized in such a way that the need for any destructor at all is actually quite rare (because the cleanup has been factored away into the contained object, and handled either by a standard library container or a smart pointer or perhaps one of the boost ptr_containers).




I'm more interested in the data structures, really - the headers rather than any function implementations. But I have seen enough to ask a few questions...

Apparently you do know about vectors. Why aren't you using them throughout, instead of these pairs of array and element-count? (But speaking of knowing about vectors... you know we have these lovely things called "iterators", that avoid the need to think in terms of index values? Or for that matter, standard library algorithms like std::for_each() designed to prevent you from needing to write loops iterating over a container?)

How come all your delete[]s are checked for NULL? As noted, you don't need to do this - but you seem to know that already, because none of your (scalar) deletes are checked for NULL!

Why do you have functions with names like 'Destroy'? Can't you just use the destructor? Clearly you know about destructors.

You *do* have a *virtual* destructor for Entity, right?

What's with this roundabout "finding" process for Leaf entities and dynamic_casting of them to delete them? First off - oh, that's a warning sign that you don't have that virtual destructor, isn't it. :( But anyway, aren't the Leaves all known to be contained in, well, the 'leaves' array?

What's with all these members whose names start with 'm_ptr'? Are you worried that you might somehow forget that they're pointers? Or that the compiler might somehow compile code that treats them as non-pointers? The C++ type system is quite a bit stronger than that of C, you know. (Actually, you have quite a bit of that Hungarian notation floating around, but it's very inconsistently used. Get rid of it. Please. It does nothing but add maintenance problems.)

And what kinds of things are those pointers in the User class pointing at, anyway? Does it *own* those pointed-at things? I.e. is there a separate set of whatever instances for each User? If they are sharing things, you are screwed.

In fact, if anything is sharing things, you are likely to be screwed. :) That is what boost::shared_ptr is for.

Did you really *need* to make your own hash table? Did you at least *try* std::map first to see if it was acceptable for associative lookup?

Share this post


Link to post
Share on other sites
The leaves array does not contain leaf objects. It contains bare bones data from the file. That data is used to create the leaf objects. You might say it should be deleted after the leaves are created then, but it is used in the Potentially Visible Set operations.

Quote:
Well-written code is organized in such a way that the need for any destructor at all is actually quite rare

If you can show me a different way to delete objects loaded with a level that then need to be deleted when the level is destroyed please tell me.


Quote:
Why do you have functions with names like 'Destroy'? Can't you just use the destructor? Clearly you know about destructors.


I don't *just* have all the delete code in my deconstructor, because the code in the Destroy() function ONLY destroys the currently loaded level. The class that function is in is a Service which does not get destructed until the engine is being destructed, and you want to be able to destroy a level at any time.

Yes all the destructors are virtual.

Quote:
Are you worried that you might somehow forget that they're pointers? Or that the compiler might somehow compile code that treats them as non-pointers?


No i'm not worried about the compiler treating the pointers as non-pointers. I do that so me or any one else reading the code at any point knows what type of variable a variable is with out having to dig out it's declaration.


Quote:
Does it *own* those pointed-at things? I.e. is there a separate set of whatever instances for each User? If they are sharing things, you are screwed.


Yes it does *own* these things, no, multiple user objects do not share them. The reason in fact they are pointers is because they do not have default constructors and I was unclear on the syntax of calling those *owned* objects non-default constructors during the owning classes constructor. Doing them as pointers solved that.

Quote:
Did you really *need* to make your own hash table? Did you at least *try* std::map first to see if it was acceptable for associative lookup?


Nope, I didn't *need* to make a hash table, but I don't *need* to make a 3D engine either. I had to make this hash table for a class and decided i wanted to use it since I made it.

I'm using valgrind now and pretty sure I found my problem.

[Edited by - Wavesonics on April 12, 2007 1:36:14 PM]

Share this post


Link to post
Share on other sites
Quote:
Original post by Wavesonics
Quote:
Well-written code is organized in such a way that the need for any destructor at all is actually quite rare

If you can show me a different way to delete objects loaded with a level that then need to be deleted when the level is destroyed please tell me.
The answer is that you don't delete them; this task should be left to the RAII containers that manage the objects (examples of such containers include std::vector, boost::shared_ptr, and so on).
Quote:
The reason in fact they are pointers is because they do not have default constructors and I was unclear on the syntax of calling those *owned* objects non-default constructors during the owning classes constructor. Doing them as pointers solved that.
It sounds like you were looking for 'member initializer lists'. Try Googling this term; using initializer lists where applicable should solve this particular problem for you, I think.

Share this post


Link to post
Share on other sites
Awesome thank you so much jyk, you are by far one of the most helpful people on this forum!

I'm not totally clear on how a vector would control the construction and destruction of objects, but I'll Google it.

I'm also very interested in your math library, I've been coding my own up to this point but I'm very seriously considering changing over to yours.

Is the current release considered stable?

Share this post


Link to post
Share on other sites
Quote:
Original post by Wavesonics
I'm not totally clear on how a vector would control the construction and destruction of objects, but I'll Google it.
Basically, vector is a dynamically allocated array that knows how to clean up after itself (it has many other features as well, but the fact that it manages memory allocation, reallocation, and deallocation for you is arguably its most important attribute).

A simple example:
// Manual memory management:
{
int* array = new int[10];
// Do stuff with array....
delete [] array; // Don't forget to clean up!
}

// Using std::vector:
{
std::vector<int> array(10);
// Do stuff with array....
// Clean-up occurs automatically.
}
That's a fairly trivial example, but it does illustrate the RAII principle we've been talking about.
Quote:
I'm also very interested in your math library, I've been coding my own up to this point but I'm very seriously considering changing over to yours.

Is the current release considered stable?
Thanks for the interest :-) I'll PM you in a bit with some information on the library.

Share this post


Link to post
Share on other sites
Ok but, with vectors.

If I need to instantiate an object into one. How would I do this? Right now I use them to store pointers to objects which is fine.

But If I do something like:

vector<SomeClass*> v;

v.push_back( new SomeClass( someArg ) );

Would I replicate that as:

vector<SomeClass> v;

v.push_back( SomeClass( someArg ) ); ?

And would it be safe to pass around a pointer to that?

Share this post


Link to post
Share on other sites
Quote:
Original post by Wavesonics
Ok but, with vectors.

If I need to instantiate an object into one. How would I do this? Right now I use them to store pointers to objects which is fine.

But If I do something like:

vector<SomeClass*> v;

v.push_back( new SomeClass( someArg ) );

Would I replicate that as:

vector<SomeClass> v;

v.push_back( SomeClass( someArg ) ); ?

And would it be safe to pass around a pointer to that?
The first thing to recognize is that this:
vector<SomeClass*> v;
v.push_back( new SomeClass( someArg ) );
And this:
vector<SomeClass> v;
v.push_back( SomeClass( someArg ) );
Do two different things. Both are perfectly valid, but they're not interchangeable.

Let me try to give a more detailed example (this post might get kind of long).

One way to go about making the improvements to your design that have been suggested is to identify the new/delete pairs in your code, and then replace them with appropriate RAII containers.

Let's work with your first example, above, but go back to a 'manual' version. It might look something like this (all examples typed into post, so no guarantees of correctness):
    Object** objects = new Object*[number_of_objects];
for (int i = 0; i < number_of_objects; ++i) {
objects[i] = new Object(some_arg);
}

// Do a bunch of stuff...

for (int i = 0; i < number_of_objects; ++i) {
delete objects[i];
}
delete [] objects;
There are two new/delete pairs here. We'll replace them one at a time.

The first new/delete pair is used to create and destroy a dynamic array of Object*'s. The first thing that should spring to mind when presented with such an array is std::vector, so we'll revise the code as follows:
    std::vector<Object*> objects(number_of_objects);
for (int i = 0; i < objects.size(); ++i) {
objects[i] = new Object(some_arg);
}

// Do a bunch of stuff...

for (int i = 0; i < objects.size(); ++i) {
delete objects[i];
}
//delete [] objects; // No longer necessary!
Now let's look at the other new/delete pair, which is used to create and destroy the individual Object's. For this particular case, the most straightforward option is probably boost::shared_ptr. Let's revise again:
    std::vector<boost::shared_ptr<Object> > objects(number_of_objects);
for (int i = 0; i < objects.size(); ++i) {
objects[i].reset(new Object(some_arg));
}

// Do a bunch of stuff...

// No longer necessary! When the shared_ptr objects are destroyed
// (which happens automatically when the vector object goes out of
// scope), they automatically delete their held resources.
//for (int i = 0; i < objects.size(); ++i) {
// delete objects[i];
//}
In summary, we've reduced this:
    Object** objects = new Object*[number_of_objects];
for (int i = 0; i < number_of_objects; ++i) {
objects[i] = new Object(some_arg);
}

for (int i = 0; i < number_of_objects; ++i) {
delete objects[i];
}
delete [] objects;
To this:
    std::vector<boost::shared_ptr<Object> > objects(number_of_objects);
for (int i = 0; i < objects.size(); ++i) {
objects[i].reset(new Object(some_arg));
}
It should be clear that, applied consistently, this approach to management of dynamic resources drastically reduces code size and eliminates many opportunities for error. Basically, a good goal to shoot for is to have zero calls to delete in your own code, only use new to create single dynamically allocated resources, and immediately pass ownership of such a resource to a smart pointer object.

It looks like I kind of got sidetracked and didn't actually answer your original question, but that'll have to wait for another post :)

Share this post


Link to post
Share on other sites
Quote:
Original post by Wavesonics
The leaves array does not contain leaf objects. It contains bare bones data from the file. That data is used to create the leaf objects. You might say it should be deleted after the leaves are created then, but it is used in the Potentially Visible Set operations.


Why not store it in the leaf objects, if it's part of the information needed to perform operations pertaining to leaves?

Quote:
Quote:
Well-written code is organized in such a way that the need for any destructor at all is actually quite rare

If you can show me a different way to delete objects loaded with a level that then need to be deleted when the level is destroyed please tell me.


jyk is covering this pretty well so far...

Quote:
Quote:
Why do you have functions with names like 'Destroy'? Can't you just use the destructor? Clearly you know about destructors.


I don't *just* have all the delete code in my deconstructor, because the code in the Destroy() function ONLY destroys the currently loaded level.


This sounds like the Level should be an object, which cleans itself up in the destructor, and the BSPManagerService would then manage a collection of Level objects instead of several collections of assorted stuff.

In general, it looks like you are trying to put too much data into your classes, which results in extra complexity. The natural way to break things up is to add more class types.

Quote:
The class that function is in is a Service which does not get destructed until the engine is being destructed, and you want to be able to destroy a level at any time.


You don't need classes to represent such things - I assume you're using some kind of Singleton pattern to model this? :(

Quote:
Quote:
Are you worried that you might somehow forget that they're pointers? Or that the compiler might somehow compile code that treats them as non-pointers?


No i'm not worried about the compiler treating the pointers as non-pointers. I do that so me or any one else reading the code at any point knows what type of variable a variable is with out having to dig out it's declaration.


With simplified code, it's a lot easier to find the declarations, and fewer things are pointers, so these problems tend to solve themselves. But regardless, what's the worst thing that happens if someone forgets? S/he assumes a non-pointer, gets a compiler error and then fixes it.

Now, how does that compare to what happens if you forget to call something a 'ptr', or forget to change something back from being a 'ptr' if you work out that you no longer need a pointer? The client now reads the name, comes to the wrong conclusion, and still gets a compiler error to fix. (And in fact, I'll note that you're *not* consistent about how you use 'ptr' in *local* variables, at least.) Now, how does the maintenance work compare, even for hypothetical people who never forget things? None versus some. Now, how does the compile time compare when maintenance is performed? None versus probably quite a bit.

(Those arguments naturally apply to all the other Hungarian notation as well. And you've been inconsistent about using the other Hungarian notation, as well. This is to be expected; it's *not natural*, so you have to force yourself into it, and will forget it on occasion.)

But beyond that - Use just about anything other than a pointer when you can, and raw pointers only when you really need to. C++ is designed to let you do this in several ways, often cleaning up the look of code immensely, and often at no or even negative cost (because, by being more specific, you can give more information to the compiler, which can then perform an optimization which it would otherwise have ruled out as unsafe).

For example (from removeElementById(), with some stuff cleaned up and reformatted):


ClassRegistryEntry *ClassEntry = m_ElementsByClassTable->retrieve( Element->getClassName());

if (ClassEntry->m_elements.size() > 0) {
bool found = false;
for (int i = 0; i < ClassEntry->m_elements.size() && !found; ++i) {
if (ClassEntry->m_elements.at(i) == ElementId) {
ClassEntry->m_elements.erase(ClassEntry->m_elements.begin() + i);
found = true;
}
}
if (ClassEntry->m_elements.size() == 0) {
m_ElementsByClassTable->removeByKey(ClassEntry->m_ClassName);
}
} else {
LOG->writeMsg( Logger::LVL_ERROR, "Element Manager Service: Element Class entry was empty before delete was preformed" );
}



Here, since the pointer returned from retrieve() is not checked, I assume it always needs to be valid. Therefore, we could return a reference instead, and write the code more simply. (If it *could* be a NULL pointer, then you can still return a reference, and throw an exception in the not-found case. This will have the benefit of crashing your program in a *controlled* way if that condition is not checked for, instead of *invoking undefined behaviour*.)

But then, we only ever work with the m_elements (several times) and m_strClassName (just once) of the ClassEntry, so let's make a reference to the m_elements as well. (Smart C programmers used to refactor things by making a pointer to the nested member, but that's kind of tricky because you have to watch your &'s and *'s carefully.)


ClassRegistryEntry& ClassEntry = m_ElementsByClassTable->retrieve( Element->getClassName());
// assuming retrieve() now returns a reference.

vector<string>& elements = ClassEntry.m_elements;

if (elements.size() > 0) {
bool found = false;
for (int i = 0; i < elements.size() && !found; ++i) {
if (elements.at(i) == ElementId) {
elements.erase(elements.begin() + i);
found = true;
}
}
if (elements.size() == 0) {
m_ElementsByClassTable->removeByKey(ClassEntry.m_strClassName);
}
} else {
LOG->writeMsg( Logger::LVL_ERROR, "Element Manager Service: Element Class entry was empty before delete was preformed" );
}



By making a reference, we get to work with the value as if we made a copy, except that we work on the actual value instead of a copy. It provides another, more convenient name for the thing we're interested in - very convenient. And safer. And comes at no overhead vs. a pointer.

And speaking of references, object types (such as std::string) should normally be passed to functions by const reference. Exceptions are commonly made when:

a) The class is int-sized, or is some kind of iterator.
b) The function is going to make a copy *anyway*; passing by value then makes the copy implicitly.

Quote:
Quote:
Does it *own* those pointed-at things? I.e. is there a separate set of whatever instances for each User? If they are sharing things, you are screwed.


Yes it does *own* these things, no, multiple user objects do not share them. The reason in fact they are pointers is because they do not have default constructors and I was unclear on the syntax of calling those *owned* objects non-default constructors during the owning classes constructor. Doing them as pointers solved that.


Like jyk said. You should be using initializer lists *anyway* where you can. The solution to lack of familiarity with a language feature is not to change your fundamental design to avoid it. Especially not when it adds code *and* memory overhead *and* very likely, performance overhead.

Quote:
Quote:
Did you really *need* to make your own hash table? Did you at least *try* std::map first to see if it was acceptable for associative lookup?


Nope, I didn't *need* to make a hash table, but I don't *need* to make a 3D engine either. I had to make this hash table for a class and decided i wanted to use it since I made it.


I would say that just about the first thing any *good* class should teach you is not to use stuff you made yourself for basic tasks when there is something equivalent in the standard library. It's the "standard" library for a reason, after all. Of course, I doubt there are very many good programming classes out there. In fact, I'm pretty sure there are very few.




Anyway, speaking about iterators etc.:

- .at() on a vector is bounds-checked, whereas operator[] (a) normally isn't, and (b) is more idiomatic (you treat the vector like an array, which makes sense because it effectively is a wrapper for an array). If you're generating the indices yourself, you really don't need bounds checking.

- But we don't need to treat iterators like this. It seems as though working in C and/or C++ *before* std::vector has damaged you to the point where working with collections like this seems natural - where "for each number i in the range of the number of things, do something with the ith thing" makes more sense than "for each thing in the collection, do something with it". We get the second behaviour by proper use of iterators. Notice how .erase() accepts an iterator? The standard library is *expecting* you to work with iterators "natively" instead of converting back and forth between iterators and array-indices. (Of course, it sort of *has* to do that, so that containers like std::list can provide basically the same interface that std::vector does.) When you write 'container.begin() + i', that *doesn't work* with a std::list; the iterator is designed to make you use a special function instead for that kind of conversion, to highlight the fact that such a conversion is *not efficient* (because of the properties of a std::list). If you iterate with iterators, you already have the iterator you need as soon as you find the element.

- But we don't need to write loops like this. (Also, 'break'ing out of a for loop is perfectly ok anyway.) The standard library provides for these common cases anyway, with algorithms like std::find().

- Don't compare the .size() of a container to 0 anyway. .empty() is idiomatic, and also may be more efficient (for some implementations of std::list).


ClassRegistryEntry& ClassEntry = m_ElementsByClassTable->retrieve( Element->getClassName());

vector<string>& elements = ClassEntry.m_elements;

if (!elements.empty()) {
vector<string>::iterator it = find(elements.begin(), elements.end(), ElementId);
if (it != elements.end()) {
elements.erase(it);
if (elements.empty()) {
m_ElementsByClassTable->removeByKey(ClassEntry.m_strClassName);
}
}
} else {
LOG->writeMsg( Logger::LVL_ERROR, "Element Manager Service: Element Class entry was empty before delete was preformed" );
}



The name 'it' is of course short for "iterator", but it also reads as the pronoun 'it' in many cases. :) Anyway, you can see now how much clearer this is. We find things using find(). We check if something is empty with its .empty(). We erase *the thing that was found* (i.e., the return value from std::find()), if it's found. And we never have to worry about pointers for things that don't really involve pointers conceptually.

Share this post


Link to post
Share on other sites
If you want a good debugger for Linux, i strongly suggest DDD. Basically GDB with a GUI. In some ways it is actually nicer than the visual studio debugger too, and it provides alot of the features.

Dave

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this