I have a robot class that has a pointer vector of ints (to store work done history), however when I copy an object of one robot to another and the first robot goes out of scope, and then I print the history of the robot it gives me a massive list of random numbers. I ve tried making my own copy constructor and setting _history to new objects _history value by value, but gives same response.
ROBOT.h
# pragma once
#include <iostream>
#include <vector>
class Robot{
private:
int workUnit = 0;
std::vector<int>* _history; // pointer to vector of ints (NOT a vector of int pointers)
public:
Robot() : name("DEFAULT") {_history = new std::vector<int>();};
Robot(const std::string& name) : name(name){_history = new std::vector<int>();};
~Robot(){std::cout << name << ": Goodbye!" << std::endl; delete _history;};
std::string whoAmI() const {return name;};
void setName(const std::string& name){this->name = name;};
void work();
void printWork() const;
std::vector<int>* getHistory() const { return _history; };
protected:
std::string name;
};
ROBOT.cpp
# include "Robot.h"
void Robot::work(){
workUnit ;
_history -> push_back(workUnit);
std::cout << name << " is working. > " << workUnit <<"\n";
}
void Robot::printWork() const {
std::cout << "Robot " << name << " has done the following work: ";
for(const int& record : *_history){
std::cout << record << " ";
}
std::cout << std::endl;
}
MAIN
#include <iostream>
#include "Robot.h"
int main(){
Robot r2("Task5 Robo");
{
Robot r1("r1");
r1.whoAmI();
r1.work();
r1.work();
r1.printWork();
std::cout << "assign r1 to r2..." << std::endl;
r2 = r1;
r2.setName("r2");
r2.whoAmI();
r2.printWork();
}
r2.whoAmI();
r2.printWork();
std::cout << "end of example code..." << std::endl;
return 0;
}
OUTPUT iam getting :
r1 is working. > 1
r1 is working. > 2
Robot r1 has done the following work: 1 2
assign r1 to r2...
Robot r2 has done the following work: 1 2
r1: Goodbye!
Robot r2 has done the following work: 7087248 0 6975376 0 0 0 -1124073283 19523 7087248 0 6975408 0 7087248 0 6947152 0 0 -1 -1174404934 19523 7087248 0 6947152 0 1701603654 1917803635 1701602145 1986087516 1634360417 (and lots more random numbers)
CodePudding user response：
Here's one example of how to implement the five special member functions in the rule of 5. First, your default constructor and the constructor taking a string could be combined so that the default constructor delegates to the one taking a string:
Robot(const std::string& name) :
_history(new std::vector<int>()),
name(name)
{};
Robot() : Robot("DEFAULT") {} // Delegate
Here's the rule of five:
// --- rule of five ---
Robot(const Robot& rhs) : // copy constructor
workUnit(rhs.workUnit),
_history(new std::vector<int>(*rhs._history)),
name(rhs.name)
{}
Robot(Robot& rhs) noexcept : // move constructor
workUnit(rhs.workUnit),
// use exchange to steal pointer and replace with nullptr:
_history(std::exchange(rhs._history, nullptr)),
name(std::move(rhs.name))
{}
Robot& operator=(const Robot& rhs) { // copy assignment operator
workUnit = rhs.workUnit;
*_history = *rhs._history; // use vector's copy assignment operator
name = rhs.name;
return *this;
}
Robot& operator=(Robot&& rhs) noexcept { // move assignment operator
workUnit = rhs.workUnit;
// swap pointers, let rhs destroy *this old pointer:
std::swap(_history, rhs._history);
name = std::move(rhs.name);
return *this;
}
~Robot() { // destructor
std::cout << name << ": Goodbye!\n";
delete _history;
};
// --- rule of five end ---
When you are dealing with raw owning pointers, you could use a std::unique_ptr to get some of this for free though. Instead of
std::vector<int>* _history;
you make it:
std::unique_ptr<std::vector<int>> _history;
So the class becomes:
Robot(const std::string& name) :
_history(std::make_unique<std::vector<int>>()),
name(name)
{};
Robot() : Robot("DEFAULT") {} // Delegate
And the rule of five becomes a little simpler:
// --- rule of five ---
Robot(const Robot& rhs) : // copy constructor
workUnit(rhs.workUnit),
_history(std::make_unique<std::vector<int>>(*rhs._history)),
name(rhs.name)
{}
// move constructor handled by unique_ptr, just default it:
Robot(Robot& rhs) noexcept = default;
Robot& operator=(const Robot& rhs) { // copy assignment operator
workUnit = rhs.workUnit;
*_history = *rhs._history;
name = rhs.name;
return *this;
}
// move assignment operator handled by unique_ptr, just default it:
Robot& operator=(Robot&& rhs) noexcept = default;
~Robot() { // destructor
std::cout << name << ": Goodbye!\n";
// delete _history; // no delete needed, unique_ptr does it
};
// --- rule of five end ---
You may also want to return your _history by reference from getHistory(). The below works for both the raw pointer and the unique_ptr version and gives a nicer interface to your class:
const std::vector<int>& getHistory() const { return *_history; };
std::vector<int>& getHistory() { return *_history; };
CodePudding user response：
When you destroy a robot, you destroy its work history. What happens when you copy a robot, is that it gets a copy of the pointer to the work history. In other words, the second robot has a pointer to exactly the same vector of integers that the first robot created.
Now when the first robot is destroyed, it deletes the work history that it owns. This is why the second robot's work history is invalid when printed: that memory has been freed up.
I can suggest two possible solutions to this. One is to implement the "Rule of 5" which, among other things, would allow you to specify (by defining a copy constructor and an assignment operator) how one robot could make a copy of another robot, including creation of a work history that it would own and which could not be deleted by the first robot. The other is to use a "Shared pointer" to manage the lifetime of the work history.
Given that a work history sounds like something that should not be shared by multiple robots, I'd go for the first option.
CodePudding user response：
r2 = r1;
Uses the implicitly declared defaulted copy assignment operator. Since the default implementation simply does a memberwise copy, the old _history pointer is simply overwritten and the vector is unexpectedly owned by 2 objects in addition to the old vector stored in r2 before the assignment not being freed properly.
You should implement move constructor move assignement operator, copy constructor copy assignement operator or both pairs.
It's only necessary to do this though, if you keep the _history vector a raw pointer; changing to std::vector<int> the default implementations of the constructors/assignment operators are present and working, changing to std::unique_ptr<std::vector<int>> would result in the copy assignment operator/copy constructor being deleted.
Note: For all approaches you should change the return types of whoAmI and getHistory as described in the last option.
Copy assignment operator
This keeps both objects "intact" after the assignment.
A custom implementation to properly copy the pointer is necessary.
class Robot{
...
public:
...
Robot(Robot const& other)
: workUnit(other.workUnit), _history(new std::vector<int>(*other._history)), name(other.name)
{}
Robot& operator=(Robot const& other)
{
workUnit = other.workUnit;
*_history = *other._history;
name = other.name;
return *this;
}
...
};
Move assignment
This leaves requires you to change the assignment to r2 = std::move(r1); leaving r1 in an state where only the destructor is guaranteed to work. Note that doing this you shouldn't print name in the destructor, since r2's name has been moved from.
class Robot{
...
public:
...
Robot(Robot && other) noexcept
: workUnit(other.workUnit), _history(other._history), name(std::move(other.name))
{
other._history = nullptr; // prevent double free; we're the sole owner of the vector now
}
Robot& operator=(Robot && other) noexcept
{
workUnit = other.workUnit;
delete _history; // old history no longer needed -> free to avoid memory leak
_history = other->_history;
other._history = nullptr; // prevent double free; we're the sole owner of the vector now
name = std::move(other.name);
return *this;
}
...
};
The simple option (Recommended)
Go with std::vector<int> and go with the defaulted constructors:
class Robot{
private:
int workUnit = 0;
std::vector<int> _history; // pointer to vector of ints (NOT a vector of int pointers)
public:
Robot() : name("DEFAULT") {}
Robot(const std::string& name) : name(name){}
~Robot(){std::cout << name << ": Goodbye!" << std::endl; }
Robot(Robot const&) = default;
Robot& operator=(Robot const&) = default;
// don't print name in the destructor, if you uncomment the following 2 members
// Robot(Robot&&) = default;
// Robot& operator=(Robot&&) = default;
std::string const& whoAmI() const {return name;} // user should be able to decide, if a copy is needed
void setName(const std::string& name){this->name = name;}
void work();
void printWork() const;
std::vector<int> const& getHistory() const { return _history; } // don't return raw a pointer here
std::vector<int>& getHistory() { return _history; } // overload only needed, if the history needs to be modifiable from the outside
protected:
std::string name;
};
If the history returned needs to be modifiable for a const object, consider making _history mutable.