For a program I'm writing, I wrote a simple array wrapper class (the idea is that it's supposed to be fixed-size. I know I could just use std::vectors)
And I have an issue when deleting the Array.
Those are my constructors:
template <class T>
Array<T>::Array(size_t size): m_data(new T[size]), m_size(size)
{}
template <class T>
Array<T>::Array(const std::vector<T> &vec): m_size(vec.size())
{
std::allocator<T> a;
m_data = a.allocate(m_size);
for(int i = 0; i<m_size; i++)
{
new (m_data+i) T(vec[i]);
}
}
And this is my destructor:
template <class T>
Array<T>::~Array()
{
delete[] m_data;
}
I used valgrind to try to figure out what's going on, but it didn't help.
==20840== Invalid read of size 8
==20840== at 0x10ABB8: sapph_dijkstra::Array<sapph_dijkstra::Node>::~Array() (in /home/sapphie/dijkstra/test)
==20840== by 0x1091CF: sapph_dijkstra::MinHeap::~MinHeap() (minheap.h:40)
==20840== by 0x109021: main (test.c:20)
==20840== Address 0x5b21318 is 8 bytes before a block of size 400 alloc'd
==20840== at 0x4C3017F: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==20840== by 0x109F99: __gnu_cxx::new_allocator<sapph_dijkstra::Node>::allocate(unsigned long, void const*) (new_allocator.h:111)
==20840== by 0x10AA36: sapph_dijkstra::Array<sapph_dijkstra::Node>::Array(std::vector<sapph_dijkstra::Node, std::allocator<sapph_dijkstra::Node> > const&) (in /home/sapphie/dijkstra/test)
==20840== by 0x10A232: sapph_dijkstra::MinHeap::MinHeap(std::vector<sapph_dijkstra::Node, std::allocator<sapph_dijkstra::Node> > const&) (in /home/sapphie/dijkstra/test)
==20840== by 0x108FD1: main (test.c:20)
==22059== Invalid free() / delete / delete[] / realloc()
==22059== at 0x4C3173B: operator delete[](void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==22059== by 0x10ABA4: sapph_dijkstra::Array<sapph_dijkstra::Node>::~Array() (in /home/sapphie/dijkstra/test)
==22059== by 0x1091CF: sapph_dijkstra::MinHeap::~MinHeap() (minheap.h:40)
==22059== by 0x109021: main (test.c:20)
(I've cut some identical error messages)
In the prints I've done, I've figured out that the address I allocated is at 0x5b21320, so indeed the address shown by valgrind is 8 bytes before that.
But I don't understand why. I don't seem to access that.
Is there something trivial I'm missing?
I realise that this is probably an overly complicated solution for a problem where I could just use a standard vector, and I'm probably gonna change to that. But I'm mostly curious now.
You can't delete using delete[] if the memory wasn't initialized using new[]. This is one situation (of very, very, very few situations) where it's correct to directly call an object's destructor:
template <class T>
Array<T>::~Array()
{
for(size_t i = 0; i < m_size; i++) {
m_data[i].~T();
}
allocator.deallocate(m_data);
}
There's a few other changes you should probably make:
Array object, not a local object in its constructor. While most allocators are not stateful, some are, and having to create a local copy of the allocator only within the constructor means that your allocator is going to lose its state.delete[] destroys objects. In your situation, you need to specify that yourself, by altering the order of the for-loop in the destructor.This is what that would look like:
template <class T>
Array<T>::~Array()
{
for(size_t i = 0; i < m_size; i++) {
m_data[m_size - i - 1].~T();
}
allocator.deallocate(m_data);
}
int for anything involving sizes of this array. There's an argument to be made to prefer int64_t over size_t (though that would break from how the STL behaves, and is a decision that would need to be made carefully), but you definitely don't want to cap yourself to using a signed 32-bit number or smaller (which int is on most environments).delete:delete expects the pointer passed to it to be its own discretely allocated object. Objects created using placement-new are not allocated this way. Consider the following:
struct point {
int32_t x, y;
};
int main() {
char memory[1024];
size_t size = 128;
point * arr = reinterpret_cast<point*>(memory);
for(size_t i = 0; i < size; i++) {
new(memory + i) point();
}
for(size_t i = 0; i < size; i++) {
arr[i].x = 5;
arr[i].y = 10;
}
for(size_t i = 0; i < size; i++) {
//undefined behavior, we're deleting objects that weren't allocated on the heap!
delete (arr + (size - i - 1));
}
}
In this example, there's no dynamic allocation of memory at all; all memory used is allocated on the stack. If we used delete in this situation, we'd be deleting pointers to memory on the stack, and any number of (undefined) things could happen, most likely including crashing our program. Calling the destructor allows the object to clean up its component objects without needing to perform the (in this case, unnecessary) deallocation of memory.
for(size_t i = 0; i < size; i++) {
//Correct behavior, doesn't delete stack memory
arr[size - i - 1].~point();
}
Even in the situation where this memory was allocated on the heap, we'd still be deleting pointers that weren't expressly allocated by the code. That's forbidden by the language, and it's easy to see why: a single deallocation of the original memory erases the whole chunk of allocated memory, there's no need to deallocate the individual chunks.
struct point {
int32_t x, y;
};
int main() {
char * memory = new char[1024];
size_t size = 128;
point * arr = reinterpret_cast<point*>(memory);
for(size_t i = 0; i < size; i++) {
new(memory + i) point();
}
for(size_t i = 0; i < size; i++) {
arr[i].x = 5;
arr[i].y = 10;
}
for(size_t i = 0; i < size; i++) {
//Still UB: only the first object is a discrete allocation, the rest are part of
//that original allocation. This attempts to deallocate the same chunk of memory 128 times
//delete (arr + (size - i - 1));
//This is correct
arr[size - i - 1].~point();
}
//We do need to deallocate the original memory allocated, but we only do this once.
delete[] memory;
}