How to properly allocate and deallocate a map of vectors

Question

I am trying to keep data in a unordered map of vectors, using uint32_t addresses as keys. The vector need to be stored in the heap so I can still access the data even if the function goes out of scope. I did the allocation like this:

using vectorBuffer = std::vector<uint8_t>;
using I2cVectorMap = std::unordered_map<address_t, vectorBuffer>;
using I2cVectorEntry = std::pair<address_t, vectorBuffer>;

if(auto i2cIter = i2cVectorMap.find(address); i2cIter == i2cVectorMap.end()) {
    vectorBuffer * receiveVector = new vectorBuffer(maxReplyLen);
    i2cVectorMap.insert(I2cVectorEntry(address, *receiveVector));
}
else {
    // Already in map. readjust size
    vectorBuffer * existingVector = &i2cIter->second;
    existingVector->resize(maxReplyLen);
    existingVector->shrink_to_fit();
}

And the deallocation like this (as recommended by various sources, swapping the vector with an empty one):

// getting the address from another object.
address_t deviceAddress = i2cCookie->getAddress();
if(auto i2cIter = i2cVectorMap.find(deviceAddress); i2cIter != i2cVectorMap.end()) {
    vectorBuffer().swap(i2cIter->second);
}

is this a correct implementation? I will pass the address to the map entry either to a driver function initiating a I2C transfer or to another object processing the data. I want to ensure that I don't have a memory leak.

Thanks a lot in advance !

Your map holds full vectors - so creating a new map entry will *copy* that vector into the map. As written, your code has a memory leak. You dynamically create a `vectorBuffer` on the heap, copy its contents into the map, and then lose the pointer to the memory on the heap. You likely want to `emplace` your `vectorBuffer` in the map. Then you don't need to worry about memory management at all. https://en.cppreference.com/w/cpp/container/unordered_map/emplace — JohnFilleau, Mar 20 '20 at 15:44
General point: You almost never want to dynamically allocate a C++ Standard library container. They work their best magic managing themselves. Side note: `vectorBuffer * existingVector = &i2cIter->second;` could be `vectorBuffer & existingVector = i2cIter->second;` It's not all that important here, but if you needed to `answer = existingVector[42];`, the pointer makes using the `[]` operator tragically difficult. — user4581301, Mar 20 '20 at 15:56

walnut · Accepted Answer · 2020-03-20T16:17:53.853

No, you are not supposed to use new. The cases where direct news are required are rare in the first place.

You don't need to create any objects with dynamic storage duration ("on the heap") manually. The standard library containers store objects directly, not references or pointer to objects. They will allocate for these objects and construct them by themselves. You only provide constructor arguments to the container member functions from which the container (copy/move) constructs the stored elements.

You can just the declare the object as automatic variable (i.e. "on the stack"):

vectorBuffer receiveVector(maxReplyLen);
i2cVectorMap.insert(I2cVectorEntry(address, receiveVector));

or use a temporary for the intermediate vector instance instead of a named variable (this also will use a move instead of a copy operation and is therefore better for performance):

i2cVectorMap.insert(I2cVectorEntry(address, vectorBuffer(maxReplyLen)));

and if you use emplace instead of insert, you can drop the redundant pair type as well:

i2cVectorMap.emplace(address, vectorBuffer(maxReplyLen));

You don't need a pointer to call methods on the object in the map:

vectorBuffer& existingVector = i2cIter->second;
existingVector.resize(maxReplyLen);
existingVector.shrink_to_fit();

And to remove an element from the map, you use its .erase member function:

i2cVectorMap.erase(deviceAddress);

You don't need to do anything else. This will destruct and free the vector as well.

When you don't need the map and the vectors it contains anymore, there is also nothing you need to do. The destructor of the map will clean up everything automatically when the scope in which the map is defined is left.

Thanks a lot ! I incorporated the changes. I have never seen the address operator on the left side. Does this approach have some advantages? (I guess this was partially answered by user4581301) — Spacefish, Mar 20 '20 at 16:02
@Spacefish This is the syntax for defining a *reference*. That is a very very fundamental language construct in C++. If you haven't heard of it before, you need to have a look at a [good introductory book](https://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list). — walnut, Mar 20 '20 at 16:08
@Spacefish If you're already familiar with the likes of C and other C-derived languages [here's a very terse "book"](https://isocpp.org/tour) to get you started. If the depth and pace of the information it tosses at you scares you into getting more books, I see this as a good thing. C++ is crazy complicated, and just figuring out what features you want to use and which you don't can take a lot of reading. — user4581301, Mar 20 '20 at 16:21
I am still relatively new to C++, and especially to the modern C++ language features, so thanks a lot for the recommendations. — Spacefish, Mar 20 '20 at 16:23

How to properly allocate and deallocate a map of vectors

1 Answers1