Memory deallocation, am I doing it right?

[tkhan0301]

Hello all,

I've been in the programming world for just few years, and I've been using Java for the most part of it.

I started studying c++ by my own just recently, and I still don't feel comfortable with deleting pointers.

So I would appreciate if someone could review my code and tell me if I'm doing the right things.

First of all, my constructor goes like this:

template <typename T>
Array3<T>::Array3(const sf::Vector3s& size) :
m_size(size)
{
	m_array = new T**[m_size.x];
	for (int i = 0; i < m_size.x; i++)
	{
		m_array[i] = new T*[m_size.y];
		for (int j = 0; j < m_size.y; j++)
		{
			m_array[i][j] = new T[m_size.z];
		}
	}
}

Just for your information, sf::Vector3<T> is a simple class from SFML containing T x, T y, T z.

And finally, my destructor goes like this:

template <typename T>
Array3<T>::~Array3()
{
	for (int i = 0; i < m_size.x; i++)
	{
		for (int j = 0; j < m_size.y; j++)
		{
			delete[] m_array[i][j];
		}
		delete[] m_array[i];
	}
	delete[] m_array;
}

According the following code, is there any possible memory leak?
Anything wrong, inefficient, something about my code?

Thank you in advance.

P.S. Is there anything in STL that already implements what I did here? (I don't mean like std::vector<std::vector<std::vector<T>>>)

[Zhuge]

Your code looks good to me.

As far as I know, there is no standard container that implements a multidimensional array.

[vlad from moscow]

It seems that you are deleting allocated memory correctly.
It is std::vector that is used instead of arrays in C++.

[tkhan0301]

Thank you.
It's a relief I got the right one between delete and delete[].

[Zhuge]

Anything you new[] you must delete[], and anything you new you must delete. It's that simple. ;)

[Catfish2]

According the following code, is there any possible memory leak?

Put some objects that whine, into your Array3...

#include <iostream>

struct Whine {
    Whine() {std::clog << this << " Whine()" << std::endl;}
    ~Whine() {std::clog << this << " ~Whine()" << std::endl;}
};

If the constructor messages are matched by the destructor ones, your code is free of memory leaks. Probably.

Anything wrong, inefficient, something about my code?

I know it's a matter of style... but apart from that is there any good reason why you define the member functions outside the templated class' body?
Since you put them in the same header file anyway?

[tkhan0301]

I know it's a matter of style... but apart from that is there any good reason why you define the member functions outside the templated class' body?
Since you put them in the same header file anyway?

What I've learned is that it's better to always separate declaration and definition.
As template class cannot be defined in .cpp file, I define it in a .inl file just for the sake of separating the definition from declaration.
(Edit: I forgot to add to keyword "inline" in the code above, but I have it in the actual code)

Should I just ditch inline files and define in header file all together?

[Cubbi]

Anything wrong, inefficient, something about my code?
P.S. Is there anything in STL that already implements what I did here? (I don't mean like std::vector<std::vector<std::vector<T>>>)

What you did is exactly like vector of vectors of vectors, and it's inefficient for the same reason: each individual 1D array is allocated separately, so that any sort of iteration jumps all over the heap.
Use a single 1D vector, and provide accessors that take three indexes. Or use boost: http://www.boost.org/doc/libs/release/libs/multi_array/doc/index.html

[Catfish2]

What I've learned is that it's better to always separate declaration and definition.

I guess it is, if you're writing a library then you distribute the "definitions" as a binary blob, and the "declarations" as a header file ("interface").

Otherwise, and unless the language forces the separation, you should stick to clarity, and have fewer files (in my opinion).