CODEWITHSUNDEEP
c++vectorsmart-pointers
Samael
Samael

Reputation: 61

C++ is Unique_ptr pertinent for vectors of vectors

I'm in a case where i use a builder to use factory to create CombatUnit for my project, as show in the schema (please note that I have an abstract factory, but i wouldn't complexify it for nothing).

enter image description here

The thing is, i'm creating arrays of arrays and i'm using unique_ptr to protect from memory leaks. But i can't find if i'm overusing smart_pointer for nothing, here why

std::unique_ptr<std::vector<std::unique_ptr<std::vector<std::unique_ptr<CombatObject>>>>>& Builder_FleetsCombatObjects::BuildFleets(const std::vector<const CombatObject::ConstructionOrder_CombatObject>& orders)
{
  std::unique_ptr<std::vector<std::unique_ptr<std::vector<std::unique_ptr<CombatObject>>>>> fleets(new std::vector<std::unique_ptr<std::vector<std::unique_ptr<CombatObject>>>>());

  Factory_Mechas mFact;
  Factory_SpaceShip sFact;

  for (auto order : orders)/* No Abstract factory to match schema */
  {
    if (sFact.IsMine(order.TypeOfObjectToBuild) != false)
    {
      fleets->push_back(sFact.CreateSpaceShip(order));
    }
    else if (mFact.IsMine(order.TypeOfObjectToBuild) != false)
    {
      fleets->push_back(mFact.CreateMechas(order));
    }
  }
  return fleets;
}

From my actual perspective it's not overdoing it, even if smart_pointer are heavier and slower (in general use i mean), each unique_ptr will make sure that the sub smart_pointer "deleter" will be called.

But somehow i can't feel it's right, like i'm not understanding well how smartpointer work. Shouldn't vector "make sure" sub vector are "free"? then i could reduce to : std::unique_ptr<std::vector<std::vector<std::unique_ptr<CombatObject>>>>&

Or even std::vector<std::vector<std::unique_ptr<CombatObject>>> if vector are memory managed in c++19.

I'm not sure of what i understand, can you help me?

Upvotes: 0

Views: 723

Answers (1)

Guillaume Racicot
Guillaume Racicot

Reputation: 41770

C++ is Unique_ptr pertinent for vectors of vectors

That's for you to decide, but only rapidly looking at the code without other knowledge of the infrastructure, I would say that this is a heavy, terrible code smell. I would simply use the vector as is. It's lighter, safer, simpler, and faster.

A pointer to a STL containe is usually a strong indicator of code smell, and a pointer to a STL container that contains pointer to STL containers has even a stronger code smell. This would simply not pass review and I would urge to simplify.

Owning pointers to STL containers should practically not exist in a code, unless there is some class that encapsulate very special memory management through container pointers, but they should not escape that class. And even then, you can manage memory and reuse with simple container values.

You should give value semantics a try! It helps local thinking and usually make code simpler.

Now for the non opinion based part of the question.

Let's drop the superfluous unique pointers in your code so it's easier to explain:

std::vector<std::vector<std::unique_ptr<CombatObject>>>& Builder_FleetsCombatObjects::BuildFleets(const std::vector<const CombatObject::ConstructionOrder_CombatObject>& orders)
{
  std::vector<std::vector<std::unique_ptr<CombatObject>>> fleets{};

  Factory_Mechas mFact;
  Factory_SpaceShip sFact;

  for (auto order : orders)/* No Abstract factory to match schema */
  {
    if (sFact.IsMine(order.TypeOfObjectToBuild) != false)
    {
      fleets.push_back(sFact.CreateSpaceShip(order));
    }
    else if (mFact.IsMine(order.TypeOfObjectToBuild) != false)
    {
      fleets.push_back(mFact.CreateMechas(order));
    }
  }
  return fleets;
}

In this code, we copy and construct new vectors, and also allocate new unique pointers.

Here, your question would become: How to make sure a vector of vector of unique pointer is freed?

Well, let's start by looking at a vector of integer:

{
    std::vector<int> vec;

    vec.push_back(1);
    vec.push_back(2);
    vec.push_back(3);
}
// Did we leaked?

Here, we did not leaked. This is because the vector is responsible to allocate, and also deallocate. It will deallocate old buffers when pushing back, and will deallocate when the vector goes out of scope.

Now what about a unique pointer?

{
    std::unique_ptr<int> ptr = std::make_unique<int>(3);

    *ptr = 1;
} // Did we leaked?

Now did we leaked? No. That would be quite absurd!

Now let's try copying these two types:

std::vector<int> vec;
std::unique_ptr<int> ptr = std::make_unique<int>(3);

vec.push_back(1);
vec.push_back(2);
vec.push_back(3);

// Copy!
std::vector<int> vec2 = vec;
std::unique_ptr<int> ptr2 = ptr; // Uh oh! Error!

Here as you can see, you cannot copy the unique pointer. Copying a unique pointer won't compile.

However, you can copy a vector, so what's happening when you copy a vector of unique pointer?

std::vector<std::unique_ptr<int>> vec_ptr;

vec_ptr.push_back(std::make_unique<int>(1));
vec_ptr.push_back(std::make_unique<int>(2));
vec_ptr.push_back(std::make_unique<int>(3));

std::vector<std::unique_ptr<int>> vec_ptr2 = vec_ptr; // Error again!

Here it still won't compile! That is because to create a copy of the buffer, you need to copy each elements. If the elements cannot be copied, you cannot copy the vector, so the vector is not copiable.

Now since a vector manage the memory completely, and that unique ptr manages the memory completely, you cannot leak any memory unless you explicitly ask the unique pointer to drop ownership:

std::unique_ptr<int> ptr = std::make_unique<int>(3);
int* raw_owning_pointer = ptr.release();
// Here, I have to delete it myself

Now in your code, no matter the push backs and the unique pointer and vector combination, there is no leak!

To be really sure that there is no leak at all, use a sanitizer, or a memory debugger such as valgrind.

Upvotes: 2

Related Questions