• entries
570
2427
• views
216937

# Untitled

86 views

Uhhhhh... bugs bugs and more bugs. Shit got really messy, but I think I've cleaned most of it up. Here's a screenie of the current progress -

Yeah, I know I'm a slow coder. I have the internet to blame (code a line, browse for 10 minutes...) Anyway, can anyone see what's wrong in the below code?
// deletes a component from a map of sets.void delComponent( ComponentType* component ) {        //  THIS PART IS A MAP ( string here ) // THIS PART IS A SET //	_components[ component->getClassName() ].erase( *component );}// components are sorted in an ordered set; to change the// key you have to take it out and put it back in. This // function does that.ComponentType* refileComponent( ComponentType* component, std::string newClassName ) {	// create a new component object	ComponentType* newComponent = addComponent( newClassName );	// fill it with data from the old one	*newComponent = *component; // omg assignment operation?	// delete the old one	delComponent( component );	// return the new one	return newComponent;}

Cookies for the first person who can find the bug. Oh, and my code doesn't really have comments like that. Well, except for the "omg assignment operation?" because I wasn't 100% sure how that would be handled when I wrote it. And I also removed the fix, to add suspense.

Anyway, find that bug, and under what circumstances it will crash the system!!

EDIT: Oh, and a precondition of both functions is that the ComponentType* passed is non-NULL. Dur dur :D

Holy fuzzy monkeys in heaven, Batman... why aren't you using references!?

That string param should be a const std::string& to avoid copy overhead per call. You'd probably have a much easier time minimizing horrible memory leak problems if you refrained from using pointers all over the place and instead used references for storing these "components." Also, I sure hope that whatever a "component" is that you either have an overloaded assignment operator, or a shallow copy makes sense (which, given the proliferation of pointer types in the code you've posted, is not exactly easy to guarantee).

As it is, there's so many potential things in that code that might explode, I'm not sure which one you're handing out cookies for [wink]

About the pointer comment - yeah, I probably could be using references. I'm just more used to using pointers instead. (AndrewRussell was like "wtf store iterators noob", but har)

Nothing is allocated on the heap (aside from what STL does, which is none of my business). Nothing. Every pointer points to stack-allocated data which is maintained in an OOP fashion. I haven't really needed anything done on the heap, so I didn't use it. If I did, however, I'd be using boost::shared_ptr, so no worries there.

With respect to the string references - yeah, I'll probably change that. This isn't exactly performance intensive though, so I really didn't care too much. The actual game part of the code will be much more... optimized. Though I doubt I'll be doing much passing of stuff around anyway; there won't be nearly as much interaction with a GUI to worry about (I need to rewrite my whole GUI library, right now, its quite the mess).

The assignment operator was the first thing I checked, actually, but in this case, a shallow copy works fine. There are none of those dreaded pointers in ComponentType [wink]

But yeah, there's a real clear cut answer to this one, and it has absolutely nothing to do with pointers. Though you're getting warmer with the assignment operation ^_~

I'd guess that componentType is a base class with virtual members, and you screwed yourself with a subclass... but that's just a guess.

Whatever the problem was, I sure hope your solution involved a copy constructor [wink]

Nope, they're POD classes.

The problem lies in std::set::erase - if you attempt to "refile" a component under the same thing its listed as, you'll create two identical entries. The call to erase will actually remove both entries, thus freeing memory that you didn't want freed.

The solution was just to add in a check to make sure changes are going to be made.

lollers for shitty design.

The problem is real easy: You're using C++ [grin]

Is the assignment operation overwriting the new component's name (so that when you try to erase it, it has the old removed component's name)?

Thats the only thing that comes to mind for me.

## Create an account

Register a new account