CODEWITHSUNDEEP
c++visual-studio-2012vectorunique-ptr
zzelman
zzelman

Reputation: 425

How to set two vector<unique_ptr<...>>'s equal to one another?

I am using Visual Studio 2012 C++ and I want to set two vectors with unique pointers equal to one another.

    using namespace std;
    vector<unique_ptr<Unit>> unitVector;
    vector<unique_ptr<Unit>> nonDeadUnits;

    .... (stuff added to unitVector) ....

    for (auto unit = unitVector.begin(); unit != unitVector.end(); ++unit) {
            if ((*unit)->health > 0) {
                    nonDeadUnits.push_back(*unit);
            }
    }

    unitVector.clear();
    unitVector = nonDeadUnits; // error here (see below error code)

I want to remove all of the units that have health less than 0, but if I try and directly remove them from the vector, I attempt to access memory that I shouldn't, killing the program. That is why I have opted to do it this way. The only problem is that unique_ptr does not allow the type of copying that I want. Here is the error:

    error C2248: 'std::unique_ptr<_Ty>::operator =' : cannot access private member declared in class 'std::unique_ptr<_Ty>' c:\program files (x86)\microsoft visual studio 11.0\vc\include\xutility 2089

I would like to have unique_ptr's because the vectors call sub-class methods within a for loop later on, and it helps with overriding. So how do I set the vectors equal to one another or is there a better way?

Upvotes: 3

Views: 886

Answers (3)

Yakk - Adam Nevraumont
Yakk - Adam Nevraumont

Reputation: 275385

The fast way to do this is with remove_if and erase, but that idiom violates DRY (don't repeat yourself) and I have seen people make subtle mistakes when using it (forgetting the 2nd iterator to erase passing (inadequate) test cases, then failing in production!)

My solution to this kind of problem -- filtering a std::vector for some property -- is to write a container-based algorithm to do it for me.

template<typename SeqContainer, typename Lambda>
SeqContainer&& remove_erase_if( SeqContainer&& c, Lambda&& test ) {
  using std::begin; using std::end;
  auto new_end = std::remove_if( begin(c), end(c), std::forward<Lambda>(test) );
  c.erase( new_end, end(c) );
  return std::forward<SeqContainer>(c);
}

now that you have a container based remove_erase_if, we can filter the list:

// const & is important, we don't want to copy a `unique_ptr`
remove_erase_if( unitVector, [&]( std::unique_ptr<Unit> const& unit ) {
  return (unit->health() <= 0);
});

... and that is it. Everything with health() <= 0 is removed from the std::vector.

Other useful container based algorithms I find I use quite often include remove_erase and sort_unique_erase and binary_search. Amusingly, while the above code works std::vector, std::list and std::deque, I almost always use std::vector: but writing it to work with any sequential container is easier than writing it to work with a std::vector.

Another option for the design of these container algorithms is to take the container by value, and return it by value. This forces some std::move spam, but is basically equally efficient at runtime.

Upvotes: 1

Quuxplusone
Quuxplusone

Reputation: 27125

The general idea is to use std::remove_if to swap elements within unitsVector, and then once all the dead units are at the end of the vector, you just chop them off.

#include <memory>
#include <vector>

struct Unit {
    int health;
};

// The non-working version.
//
// void remove_dead_units(std::vector<std::unique_ptr<Unit>> &unitVector)
// {
//     std::vector<std::unique_ptr<Unit>> nonDeadUnits;
//     for (auto unit : unitVector)
//         if (unit->health > 0)
//             nonDeadUnits.push_back(unit);
//     unitVector = nonDeadUnits;
// }

void remove_dead_units(std::vector<std::unique_ptr<Unit>> &unitVector)
{
    auto isDead = [](const std::unique_ptr<Unit> &u) -> bool { return (u->health <= 0); };
    auto newEnd = std::remove_if(unitVector.begin(), unitVector.end(), isDead);
    unitVector.erase(newEnd, unitVector.end());
}

I'm sure there are other ways to do it, hewing more closely to what you tried (EDIT: in fact KerrekSB just posted one, using only a single std::move and a swap); but I think the "shuffle and chop" method is more modern-C++ish.

Upvotes: 8

Kerrek SB
Kerrek SB

Reputation: 477010

Perhaps the following logic would be simpler:

vector<unique_ptr<Unit>> unitVector = /* ... */;
vector<unique_ptr<Unit>> nonDeadUnits;

for (auto & p : unitvector)
{
    if (p->health > 0) { nonDeadUnits.push_back(std::move(p)); }
}

unitVector.swap(nonDeadUnits);

Otherwise, the standard remove-erase idiom is probably more main-stream:

unitVector.erase(remove_if(unitVector.begin(), unitVector.end(),
                           [](unique_ptr<Unit> const & p) -> bool { return p->health <= 0; }),
                 unitVector.end());

Upvotes: 5

Related Questions