[fixed]memoryleak with irr::core::array?

[CuteAlien]

I did need so long to reproduce this bug that it's getting a little late here. So I just post it and hope someone finds the solution before me ;-)
(otherwise I will search tomorrow myself)

The following code produces a memoryleak:

Code: Select all

#define _CRTDBG_MAP_ALLOC
#include <stdlib.h>
#include <crtdbg.h>
#include <irrlicht.h>

#ifdef _IRR_WINDOWS_
#pragma comment(lib, "Irrlicht.lib")
#endif

int main()
{

	_CrtSetReportMode( _CRT_ERROR, _CRTDBG_MODE_DEBUG );
	_CrtSetDbgFlag ( _CRTDBG_ALLOC_MEM_DF | _CRTDBG_LEAK_CHECK_DF );

	struct testrow
	{
		irr::core::array<bool> items; // type in array does not matter
	};

	irr::core::array< testrow > rows;
	testrow row;
	row.items.push_back( true );
	rows.push_back( row );
	rows.insert( row, 0 );

return 0;
}

The 'crt' stuff in the code is just to detect memleaks with VisualStudio.
Adding more items or inserting more rows will produce bigger leaks.
pushing back rows (instead of using insert) won't produce leaks.

Found in an old Irrlicht, but also tested with current svn .

[hybrid]

Yes, insert has lots of calls to construct() (which is equivalent to new), but no deconstruct(), hence mem leaks. The only case where it's safe to use insert in the current form is when inserting at the end, but then push_back() is better. push_front has the same problem as it seems. I guess both should not use the construct() call for existing elements, but only for new ones.

[CuteAlien]

Ok, it took a while until I understood the allocators but I think I got it now.
construct seems to be an explicit call to the copy constructor using the already allocated memory. So the construct() call is fine, but the corresponding destruct calls for already used elements which will get overwritten is missing.

I rewrote the code now as follows and my tests seem all to work so far:

Code: Select all

	//! Adds an element at the front of the array. If the array is to small to
	//! add this new element, the array is made bigger. Please note that this
	//! is slow, because the whole array needs to be copied for this.
	//! \param element: Element to add at the back of the array.
	void push_front(const T& element)
	{
		if (used + 1 > allocated)
			reallocate(used +1);

		for (u32 i=used; i>0; --i)
		{
			//data[i] = data[i-1];
			if ( i<used)
				allocator.destruct(&data[i]);
			allocator.construct(&data[i], data[i-1]);
		}

		// data[0] = element;
		if ( used > 0 )
			allocator.destruct(&data[0]);
		allocator.construct(&data[0], element);

		is_sorted = false;
		++used;
	}


	//! Insert item into array at specified position. Please use this
	//! only if you know what you are doing (possible performance loss).
	//! The preferred method of adding elements should be push_back().
	//! \param element: Element to be inserted
	//! \param index: Where position to insert the new element.
	void insert(const T& element, u32 index=0)
	{
		_IRR_DEBUG_BREAK_IF(index>used) // access violation

		if (used + 1 > allocated)
			reallocate(used +1);

		for (u32 i=used; i>index; --i)
		{
			if ( i<used)
				allocator.destruct(&data[i]);
			allocator.construct(&data[i], data[i-1]); // data[i] = data[i-1];
		}

		if ( used > index )
			allocator.destruct(&data[index]);
		allocator.construct(&data[index], element); // data[index] = element;
		is_sorted = false;
		++used;
	}

(the reason I moved the "used++" out of the loop to the end in the insert function has nothing to do with the bug, but with the fact that this was so confusing otherwise)

edit: When looking on the above code I think it would also not hurt to just call insert(element,0) in push_front. As both are inlined doing that shouldn't cost any performance.

[hybrid]

Wouldn't a simply memmove work? All elements are already constructed with the proper allocator, only the new one needs the explicit call?! Just to avoid a huge performance penalty here, but I guess I don't know enough about the irrAllocators either.

[CuteAlien]

edit: Uhm, my first answer was mostly wrong (deleted now) :-)

It's not such a slow function. The 'new' call irritates first, because that sounds slow, but the 'new' used here does not allocate any memory, so it should be just as fast as using operator=. Still you're right and a memmove could be way faster, because the copy constructor can certainly do some slow stuff. But I guess on the other hand a copy-constructor could also be used for some real crazy stuff (like maybe registering it's own 'this' on each copy somewhere). So I'd never copy a c++ object in a general class avoiding calls to the copy constructor (even though I'd might do the same thing in my own classes).

Also it's just something you have to know as programmer when using arrays - insert's are slow. So I wouldn't worry about that so much.

[hybrid]

Yes, you're probably right, and it's at least the safe way to make all these things explicitly like it's already done for the construction.