Memory leaks and uninitialised variables

Everything about development and the OpenMW source code.
User avatar
Zini
Posts: 5538
Joined: 06 Aug 2011, 15:16

Memory leaks and uninitialised variables

Post by Zini »

Lgro recently brought to my attention that we have problems with memory leaks and uninitialised variables. The issue is currently addressed, but we shouldn't have to deal with it in the first place.

This should normally be part of any proper C++ training and we also have listed a few points in the wiki. But as a reminder I will list again some of the more important points. Please pay more attention to them, because these kinds of mistakes can be costly. We were lucky so far that nothing major bad has resulted from them.


- Every new needs to be matched by a delete.


- Do not use new[]/delete[]. Use a vector instead.


- Use new when it is needed. Common reasons are polymorphism and the need for more control about the life time of an object. If you don't need new, then avoid it. Just declaring a non-pointer member variable or local variable works fine often enough.


- Remember the rule of three (destructor, copy-constructor, assignment operator). If you have one off them, most likely you will need all three (if desired so, copy-constructors and assignment operators can be disabled instead, by declaring them private and not implementing them).


- RAII is mandatory. Classes with default constructor will initialise fine on there own (except when it is Ogre::Vector3 :( ). Most other stuff won't. If you a declare a build-in type member variable (like an int or a pointer) you need to initialise it in every single constructor of that class (usually in the initialiser list).


- Same goes for local variables. A line like

Code: Select all

int x;
is just plain bad, even if it is followed immediately by an "x = 1;".

Just reading the line above should ring the alarm bells for every seasoned C++ coder.
Yacoby
Posts: 119
Joined: 30 Sep 2011, 09:42

Re: Memory leaks and uninitialised variables

Post by Yacoby »

Heh. Thanks for pointing this out. I am guessing some of the code I am working on has this problem (It may have been me who wrote it, IDK) and it was on my tofix list given that I am working on the code again. I will try and make my code saner.

There are points where you need to use things such as OGRE_MALLOC, but in these cases you quickly hand over ownership of the variable so I suppose it doesn't matter too much.
- Every new needs to be matched by a delete.
Should we even be allowing the use of new when the object ins't directly inserted into some sort of RAII container? It adds some overhead but given the number of boost libraries etc we probably should be trying to avoid the use of raw pointers for the added safety RAII brings. I am sure there are boost smart pointers/containers for most of the situations we would ever want.

Maybe a bit late now *shrugs*
Chris
Posts: 1625
Joined: 04 Sep 2011, 08:33

Re: Memory leaks and uninitialised variables

Post by Chris »

Zini wrote:- Remember the rule of three (destructor, copy-constructor, assignment operator). If you have one off them, most likely you will need all three (if desired so, copy-constructors and assignment operators can be disabled instead, by declaring them private and not implementing them).
It might be helpful to use GCC's error attribute on ones you want disabled, too. That way you'll get a compile-time error pointing to where it's used, instead of a link-time error. It'll need to be hidden behind a macro, though, like:

Code: Select all

#ifdef __GNUC__
#define ERROR(x) __attribute__((error((x))))
#else
#define ERROR(x)
#endif

...

SoundManager(const SoundManager &rhs) ERROR("Cannot copy SoundManager objects");
- Same goes for local variables. A line like

Code: Select all

int x;
is just plain bad, even if it is followed immediately by an "x = 1;".

Just reading the line above should ring the alarm bells for every seasoned C++ coder.
Mmm, I don't know on this one. At least with local variables, you could actually hide errors since GCC will otherwise warn if a variable may be used uninitialized. If a variable has no reasonable "default" value, forcing one could hide a warning you'd otherwise get if you missed setting it somewhere you intended, nor will valgrind be able to pick up that you missed setting it, making any resulting errors hard to diagnose.
User avatar
Zini
Posts: 5538
Joined: 06 Aug 2011, 15:16

Re: Memory leaks and uninitialised variables

Post by Zini »

Should we even be allowing the use of new when the object ins't directly inserted into some sort of RAII container? It adds some overhead but given the number of boost libraries etc we probably should be trying to avoid the use of raw pointers for the added safety RAII brings. I am sure there are boost smart pointers/containers for most of the situations we would ever want.
Having a local pointer variable taking ownership of an object created by new is obviously bad, since you also get into trouble with exception safety. I still do that occasionally, but you must pay a lot of extra attention. I agree that this should be best avoided for OpenMW.

Having a pointer as a member variable is not much of a problem. You just need to remember to use the delete in the constructor (okay, there are also some exception safety aspects to consider, but they are less critical) And since we have a large code base working this way, its probably not worth to make it into a general rule.

@Chris: Well, I think you know what I think about using compiler specific features from previous discussions.
About the local variables: I don't agree. Avoiding a potential heisenbug should have priority over everything else that might in some cases help to detect logic errors.
User avatar
Zini
Posts: 5538
Joined: 06 Aug 2011, 15:16

Re: Memory leaks and uninitialised variables

Post by Zini »

Or inherit from boost::noncopyable? More crossplatform.
Never seen the point in noncopyable. Seems to be overkill (and will become redundant with C++11). As for crossplatform-ness, they are both the same.
corristo
Posts: 495
Joined: 12 Aug 2011, 08:29

Re: Memory leaks and uninitialised variables

Post by corristo »

As this thread is about best practices, I want to clarify one point: what is our policy regarding pointers/references.
When to use pointers & when to use references for class fields, method arguments? Some inconsistencies can be found in code, in one places same objects passed by pointers, in others — by references.
User avatar
Zini
Posts: 5538
Joined: 06 Aug 2011, 15:16

Re: Memory leaks and uninitialised variables

Post by Zini »

We don't have a policy about that. The OpenMW code was pretty inconsistent about that to begin with when I took over. And then there are also the various libraries we use, that also promote varying styles.
davini
Posts: 5
Joined: 21 Oct 2012, 03:26

Re: Memory leaks and uninitialised variables

Post by davini »

Does this project abide by any common or established coding standards?
User avatar
lgromanowski
Site Admin
Posts: 1193
Joined: 05 Aug 2011, 22:21
Location: Wroclaw, Poland
Contact:

Re: Memory leaks and uninitialised variables

Post by lgromanowski »

davini wrote:Does this project abide by any common or established coding standards?
https://wiki.openmw.org/index.php?title ... _Standards
Tarius
Posts: 574
Joined: 24 Oct 2011, 19:29

Re: Memory leaks and uninitialised variables

Post by Tarius »

davini wrote:Does this project abide by any common or established coding standards?
Yes, his name is Zini.
Post Reply