vector<Class> --- Question about Copy Constructor in Class

Question

Upd: Sorry if I bother somebody with such questions. I'm 48. I'm trying to get a new profession to live from. And I need little more info than 'Do this. Don't do that. And never ask why.' :) Thanks all for answering and patience with me_kind people :)

I have Class Car.

class Car {
protected:
    std::string Name;
    short Year;
    float Engine;
    float Price;
public:
Car() {
    Name = "ordinary";
    Year = 1980;
    Engine = 2.0;
    Price = 1000.;
}
Car(std::string name, short year, float engine, float price) {
    Name = name;
    Year = year;
    Engine = engine;
    Price = price;
}
Car(Car& other) {
    this->Name = other.Name;
    this->Year = other.Year;
    this->Engine = other.Engine;
    this->Price = other.Price;
}
Car(Car&& other) {
    this->Name = other.Name;
    this->Year = other.Year;
    this->Engine = other.Engine;
    this->Price = other.Price;
}
void operator= (Car& other) {
    this->Name = other.Name;
    this->Year = other.Year;
    this->Engine = other.Engine;
    this->Price = other.Price;
}

inline std::string GetName() const { return Name; }
inline short GetYear() const { return Year; }
inline float GetEngine() const { return Engine; }
inline float GetPrice() const { return Price; }
inline void SetName(std::string n) { Name = n; }
inline void SetYear(short y) { Year = y; }
inline void SetEngine(float e) { Engine = e; }
inline void SetPrice(float p) { Price = p; }
void InitCar(std::string name, short year, float engine, float price) {
    Name = name;
    Year = year;
    Engine = engine;
    Price = price;
}
void ShowCar() {
    std::cout << "Car_Name: " << Name << ";\nYear: " << Year
        << "; Engine: " << Engine << "; Price: " << Price << "\n";
    }
};

Then I create vector of Car objects.

std::vector<Car> base;

Now

base.push_back(Car());

This is OK for compiler. And next OK too:

base.push_back(Car("this_car", 1900, 1.5, 1000));

But next one NOT OK:

Car car("that_car", 2001, 3.0, 3000);
base.push_back(car);

Compiler says:

no copy constructor available

When I take of Copy Constructor from Class Car, all OK.

Could anyone explain Why I should remove Copy Constructor from the Car class?

Thanks all for help and patience.

Why are you writing your own user-defined copy, assignment, and move functions? There is no reason to. — PaulMcKenzie
– PaulMcKenzie, Commented Jul 29, 2020 at 17:49
Well, without my own user-defined copy, assignment, and move functions - all works well. But my Question is "Why I can't do it?" — Pavel Zyrianov
– Pavel Zyrianov, Commented Jul 29, 2020 at 17:51
Of course it works well, because the compiler's version of those functions are designed that way. All you're doing is making it easier for bugs to occur. — PaulMcKenzie
– PaulMcKenzie, Commented Jul 29, 2020 at 17:52
The compiler is right, there is no copy-constructor. And as a hint, your move-constructor doesn't move anything, it just copies everything. — Lukas-T
– Lukas-T, Commented Jul 29, 2020 at 17:55
That is the other disadvantage of writing your own operators when not necessary -- you lose all the advantage of the compiler being able to optimize the moves and copies. Once you step in and write your own, you're responsible for all of that work. — PaulMcKenzie
– PaulMcKenzie, Commented Jul 29, 2020 at 17:56

Remy Lebeau · Accepted Answer · 2020-07-29 17:56:27Z

The correct declaration for the copy-constructor should be:

Car(const Car& other)

Same with the copy-assignment operator, which also needs a return value of Car& as well:

Car& operator= (const Car& other)
{
    ...
    return *this;
}

Also, your move-constructor is implemented incorrectly. It is copying the Name member, not moving it. It should look like this instead:

Car(Car&& other) {
    this->Name = std::move(other.Name);
    this->Year = other.Year; other.Year = 0;
    this->Engine = other.Engine; other.Engine = 0;
    this->Price = other.Price; other.Price = 0;
}

And you need to add a move-assignment operator:

Car& operator= (Car&& other) {
    this->Name = std::move(other.Name);
    this->Year = other.Year; other.Year = 0;
    this->Engine = other.Engine; other.Engine = 0;
    this->Price = other.Price; other.Price = 0;
    return *this;
}

In which case, both copy-assignment and move-assignment operators can be implemented to use the copy-constructor and move-constructor, respectively (see What is the copy-and-swap idiom?):

Car& operator= (const Car& other) {
    Car temp(other);
    std::swap(this->Name, temp.Name);
    this->Year = temp.Year;
    this->Engine = temp.Engine;
    this->Price = temp.Price;
    return *this;
}

Car& operator= (Car&& other) {
    Car temp(std::move(other));
    std::swap(this->Name, temp.Name);
    this->Year = temp.Year;
    this->Engine = temp.Engine;
    this->Price = temp.Price;
    return *this;
}

In which case, you can combine the copy-assignment and move-assignment operators into a single implementation by passing the parameter by value instead of by reference, thus allowing the caller to decide whether to use copy or move semantics:

Car& operator= (Car other) {
    std::swap(this->Name, other.Name);
    this->Year = other.Year;
    this->Engine = other.Engine;
    this->Price = other.Price;
    return *this;
}

That being said, since std::string already implements proper copy and move semantics, you should just let the compiler generate default implementations for the copy/move-constructors and copy/move-assignment operators for you:

class Car {
protected:
    std::string Name;
    short Year;
    float Engine;
    float Price;
public:
    Car() : Car("ordinary", 1980, 2.0, 1000)
    {
    }

    Car(std::string name, short year, float engine, float price)
        : Name(name), Year(year), Engine(engine), Price(price)
    {
    }

    Car(const Car&) = default;
    Car(Car&& other) = default;

    Car& operator= (const Car&) = default;
    Car& operator= (Car&&) = default;

    ...
};

Amir Kadyrov · Accepted Answer · 2020-07-29 17:51:23Z

3

You should specify const for copy constructor parameter

Car(const Car& other) {
    this->Name = other.Name;
    this->Year = other.Year;
    this->Engine = other.Engine;
    this->Price = other.Price;
}

answered Jul 29, 2020 at 17:51

Amir Kadyrov

1,2886 silver badges9 bronze badges

2 Comments

Pavel Zyrianov Over a year ago

Yes. With const it works. Car(const Car& other) - all is Ok. May anyone explain me this or give me a link to material where to read about it more?

Remy Lebeau Over a year ago

@PavelZyrianov see en.cppreference.com/w/cpp/language/copy_constructor

Collectives™ on Stack Overflow

vector<Class> --- Question about Copy Constructor in Class

Upd: Sorry if I bother somebody with such questions. I'm 48. I'm trying to get a new profession to live from. And I need little more info than 'Do this. Don't do that. And never ask why.' :) Thanks all for answering and patience with me_kind people :)

2 Answers 2

Comments

2 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

Upd: Sorry if I bother somebody with such questions. I'm 48. I'm trying to get a new profession to live from. And I need little more info than 'Do this. Don't do that. And never ask why.' :) Thanks all for answering and patience with me_kind people :)

2 Answers 2

Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related