for (int i = 0; i < numUsed; ++i)
{
new (newElements + i) ElementType (std::move (elements[i]));
elements[i].~ElementType();
}
elements = std::move (newElements);
}
If each element contains a pointer to a MemoryBlock or any block of memory, and the element’s destructor frees the memory block, the setAllocatedSizeInternal routine destroys each element’s allocated memory by calling its destructor. This routine should not call each element’s destructor when allocating more space.
Older juce code (3.0) just called reallocate to get more memory which worked fine.
Note when I comment out the destructor call, KeyPresses are not freed.
if i replace the code with
elements.realloc((size_t)numElements);
return;
it works fine, so I don’t understand the use of the other code.
It sounds like the ElementType you’re supplying doesn’t correctly implement its copy constructor and/or move constructor. I recommend implementing these constructors correctly (slightly tricky); or using other RAII types such as unique_ptr instead of owning raw pointers, so that the compiler-generated copy and move constructors do the right thing (easier).
Realloc is only (somewhat) safe for types that are trivially copyable (see std::realloc on cppreference). Even then, it seems like object lifetimes are questionable - it’s not clear that new object lifetimes begin in the reallocated region, which may be a source of undefined behaviour.
Essentially, the above implementation is required in order to ensure correct behaviour for non-trivially-copyable types.
Maybe to explain it in a little more detail in case you or any one else is wondering what exactly is happening up here.
First, a the problem we are trying to solve: we want to make an array bigger (= allocate more RAM for it’s elements, or create a greater headroom, so we don’t have to go through this complicated step every time we add an element). This is no problem what so ever when the array is empty, complications start when the array is already filled with an arbitrary amount of elements. Since we want to store the all elements directly next to each other without any gaps of other memory in-between our addresses, the OS might just allocate a hole new address spectrum for our new sized array, as the memory directly behind our already allocated space, might be already taken by a different part of our or an entirely different application.
So … reallocating / changing the array size might mean, the address of our objects change and therefore also the memory address, where our already created elements store their data. But, we can’t just change the memory addresses without telling our elements that this happens, since the implementation of our Element might have it’s own implementation of what should happen when we need to move it in memory – and that is exactly what constructors and destructors are for. Now in most cases this is gonna be relatively easy. If we for example just store two integers, we don’t really care at all where these integers are stored exactly in memory. We just care that the values don’t change when they are reallocated/copied to a new address. That is exactly, what trivially copyable means. Essentially the default copy-constructor, that just copies all bytes over to the new address. And since the layout of our data doesn’t change (i.e. first int is the first four bytes, second int the bytes 5-8) our data still makes perfect sense.
The code above is necessary, when this trivially copyable type trait is not met for a specific type, like for example std::unique_ptr. So using the unique_ptr as an example, here is essentially what would happen to that instance while the above code runs:
HeapBlock newElements (numElements); – we allocate memory for our new size (internally using e.g. std::malloc of some sort)
new (newElements + i) ElementType For each element we are creating a new (lets say for now empty) unique_ptr in the memory we already allocated (normally new would return the pointer to the also allocated memory, here we are essentially just invoking the constructor of ElementType using the memory we already allocated in step one)
std::move (elements[i]) – While creating, we are moving the unique_ptr currently stored into our new unique_ptr in the newly allocated memory. This move-constructor of the unique_ptr will make sure, that the ownership is transferred, so now the old unique_ptr is empty.
elements[i].~ElementType(); – at this very moment, we are sitting around with two unique_ptrs. The new one in the new memory having ownership of our resource, and the old now empty one sitting in the memory we want to get rid of. So we need to invoke the destructor of that old unique_ptr. Thats not gonna be doing anything really, because ownership is already transferred and we are just destructing an empty unique_ptr.
elements = std::move (newElements); – all elements being transferred correctly, we can now move our new memory allocation into this array making it accessible to the user. This line is also what is calling std::free on the old memory.
Essential of this operation is, that we called std::malloc and std::free the same amount of times (once) and we invoked the same amount of constructors and destructors (once for each element). So the most basic way of checking for leaked memory or resources is at satisfied.
In any case if juce Array is meant to support modern c++ you could place a if constexpr to avoid that code to be executed if elements are trivially relocatable and use realloc directly or memcpy.
I think this is already what happens. Previously this was done by std::enable_if (you can kinda see that above by the return type) but the new code base uses an if constexpr like you suggested
Thank you for the above information. This is a bit complicated to me and everything I try using the above recommendations will not compile. So why don’t I just show what I need and then maybe afterwards I’ll understand.
//Olriginal method that does not work in new C++ world.
class a
{
A() {pItems = new MemoryBlock();}
~A() {delete pItems;}
int size;
MemoryBlock *pItems;
};
How should I create the class using unique_ptr or using move/copy constructors so that I can have an array of the class?
MemoryBlock is already some kind-of RAII mechanism, so I think there is no need to use a pointer to MemoryBlock with “new” and “free” in this combination, just use its as a normal member (It always depends on what you have in mind)
So either you use MemoryBlock as a member:
class A
{
A() { pItems.setSize(20); /*or whatever you want to do*/ }
~A() { /*MemoryBlock will free memory when its destructed*/ }
int size;
MemoryBlock pItems;
};
otherwise if you insist using std::unique_ptr and a pointer to MemoryBlock (redundant twice)
class A
{
A() { pItems=std::make_unique<MemoryBlock>(); }
~A() { /* no "free" necessary, as the memory is automatically released when the pointer is destroyed. */ }
int size;
std::unique_ptr<MemoryBlock> pItems;
};
This will automatically use the the compiler-generated copy and move constructors do the right thing.
struct A
{
A() { pItems=std::make_unique<MemoryBlock>(); }
~A() { /* no "free" necessary, as the memory is automatically released when the pointer is destroyed. */ }
A(const A&) = delete;
A(A&&) = default;
std::unique_ptr<MemoryBlock> pItems;
};
This is as close as you can be to support an array. No copy of course (since the std::unique_ptr would forbid that anyway) and your move-constructor will assure that your MemoryBlock still accessible should the address of an instance of A change.
FYI: this is also a more or less performance efficient implementation, as you construct your MemoryBlock (allocate memory) only once when you actually creating A but you are still free to move A around through the memory while pItems still always points to the same memory. When you delete your A (it goes out of scope after you removed it from the array), your memory will be deallocated.
Should you want to share the ownership of pItems at a later time, to be able to pass A as a container freely around everywhere while giving up the exact knowledge of when and where your memory is actually gonna be deleted, you can use a shared_ptr and also default your copy constructor. This last bit however is an important information to know when you deal with realtime-threads, as you don’t want the realtime thread to be calling std::free as this is a system call and might or might not block.
If you still want the ability to freely pass stuff around while assuring your audio thread will never be the last one holding the reference and by that be responsible for deletion, you are looking at the newly proposed std::snapshot_ptr and std::snapshot_source proposed and hopefully scheduled for C++26. There is however a great wait-free implementation available, that was recently discussed at the WWDC23.
What ever you end up with: stay away from new and delete. C++ has a wide variety of classes that guard you properly against leaking memory. No need to use those operators anymore nowadays. And of course everything that is not covered by the standard is probably covered by JUCE.
I thank all of you for your help. I kind of understand what is going on, but I have not kept up with the new c++ methods,so I struggled with implementing the new methods. I actually did a work around that doesn’t use a pointer and it works fine…
