How add/remove to/from vector?

Question

I use unique_ptr instead of shared_ptr because it's not count reference and is faster. You can change it to shared_ptr. My question is how to add/remove properly to/from vector of smart pointers? Key lines are here:

CEO->add(std::move(headSales));
CEO->remove(headSales);

Code:

#ifndef EMPLOYEE_H
#define EMPLOYEE_H
#include <iostream>
#include <vector>
#include <memory>
class Employee
{
protected:
    std::string name;
    std::string department;
    std::uint32_t salary;
    std::vector<std::unique_ptr<Employee>> subordinates;

public:
    Employee(std::string n, std::string d, std::uint32_t s) : name(n), department(d), salary(s)
    {}
    bool operator==(const Employee &e)
    {
        return name == e.name && department == e.department && salary == e.salary;
    }
    void add(std::unique_ptr<Employee> e)
    {
        subordinates.push_back(e);
    }
    void remove(const std::unique_ptr<Employee> e)
    {
        subordinates.erase(std::remove(subordinates.begin(), subordinates.end(), e), subordinates.end());
    }
    std::vector<std::unique_ptr<Employee>> getSubordinates() const
    {
        return subordinates;
    }
};
#endif  //EMPLOYEE_H


#include "Employee.h"


int main()
{
    std::unique_ptr<Employee> CEO = std::make_shared<Employee>("John", "CEO", 30000);
    std::unique_ptr<Employee> headSales = std::make_shared<Employee>("Robert", "Head Sales", 20000);
    CEO->add(std::move(headSales));
    CEO->remove(headSales);
}

I don't understand your question... Do you have a problem, what have you tried? — Ceros, May 26 '17 at 13:31
`std::unique_ptr var = std::make_shared(/*blah*/);` ... why? — , May 26 '17 at 13:32
Possible duplicate of [Why can I not push\_back a unique\_ptr into a vector?](https://stackoverflow.com/questions/3283778/why-can-i-not-push-back-a-unique-ptr-into-a-vector) — Ceros, May 26 '17 at 13:33
Your data model is wrong it is saying: An Employee OWNS their subordinates. Think about this for a minute, if your boss leaves you get deleted. — Richard Critten, May 26 '17 at 13:36
push_back is a copy operation. unique_ptr forbids copy, as then it would not be unique. You need move. — Paul Rooney, May 26 '17 at 13:38
This code doesn't compile; make shared won't convert to a unique ptr. Why use smart pointers at all in the vector? — Yakk - Adam Nevraumont, May 26 '17 at 13:42
"I use unique_ptr instead of shared_ptr because it's not count reference and is faster." They do two rather different things; choose based on **what they do**, not on how fast one or the other may be. — Pete Becker, May 26 '17 at 13:42

score 4 · Answer 1 · answered May 26 '17 at 14:16

Your code has no need for unique or shared pointers.

#ifndef EMPLOYEE_H
#define EMPLOYEE_H
#include <iostream>
#include <vector>
#include <memory>
class Employee
{
protected:
  std::string name;
  std::string department;
  std::uint32_t salary;
  std::vector<Employee> subordinates;

public:
  Employee(std::string n, std::string d, std::uint32_t s) : name(n), department(d), salary(s)
  {}
  bool operator==(const Employee &e) const
  {
    return name == e.name && department == e.department && salary == e.salary && subordinates == e.subordinates;
  }
  void add(Employee e)
  {
    subordinates.push_back(std::move(e));
  }
  void remove(Employee const& e)
  {
    subordinates.erase(std::remove(subordinates.begin(), subordinates.end(), e), subordinates.end());
  }
  std::vector<Employee> getSubordinates() const
  {
    return subordinates;
  }
};
#endif  //EMPLOYEE_H


#include "Employee.h"


int main()
{
  Employee CEO = {"John", "CEO", 30000};
  Employee headSales = {"Robert", "Head Sales", 20000};
  CEO->add(headSales);
  CEO->remove(headSales);
}

Also your == should be const, and it should compare subordinates. Because it is now a vector of Employee, this invokes == recursively.

eerorika · Accepted Answer · 2017-05-26T14:17:18.300

std::unique_ptr<Employee> CEO = std::make_shared<Employee>("John", "CEO", 30000);
     ^^^^^^                               ^^^^^^

This is wrong because a shared pointer is not convertible to a unique pointer. It is unclear whether you intend to have shared or unique ownership.

 CEO->remove(headSales);
 subordinates.push_back(e);

These are wrong because you're trying to copy a unique pointer. Unique pointers aren't copyable however - otherwise they wouldn't remain unique. Instead, you must move from the original pointer:

subordinates.push_back(std::move(e));

Using a unique pointer as an argument by value for Employee::remove is not sensible in the first place. More about that below.

std::vector<std::unique_ptr<Employee>> getSubordinates() const
{
    return subordinates;
}

This time you're trying to copy a vector of unique pointers. The problem remains the same - the pointers are not copyable. You cannot move the vector either since the function is const. Perhaps you intend to simply give read access. That could be achieved by returning a reference:

 const std::vector<std::unique_ptr<Employee>>& getSubordinates() const

CEO->add(std::move(headSales));
CEO->remove(headSales);

You're using a pointer that you already moved from. The ownership of the unique pointer was transferred uniquely to add, so using it after the move is no longer meaningful.

As I mentioned, it makes little sense to use a unique pointer as the argument by value for remove. In order to have the unique pointer, you would have to own that pointer - uniquely. Therefore the vector couldn't possibly own it. You could instead refer to the pointer that is contained in the vector, and use that:

void remove(const std::unique_ptr<Employee>& e)

CEO->remove(CEO->getSubordinates().back());

You can change it to shared_ptr.

Well, that would be much simpler. Replace all unique with shared, and remove that one std::move and the program will be well-formed.

PS. std::remove that you want is in the <algorithm> header which you forgot to include.

PPS. It is unclear to me why an employee should have any kind of ownership of other employees - unique or shared. I suggest that all employees are owned by something else - say Company, and employees only have non-owning association. I suggest std::weak_ptr.

How add/remove to/from vector?

2 Answers2