Jump to content
  • Advertisement
Sign in to follow this  
  • entries
    570
  • comments
    2427
  • views
    216937

Untitled

Sign in to follow this  
Mushu

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
Sign in to follow this  


6 Comments


Recommended Comments

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]

Share this comment


Link to comment
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 ^_~

Share this comment


Link to comment
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]

Share this comment


Link to comment
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.

Share this comment


Link to comment
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.

Share this comment


Link to comment

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
  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!