0

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.

Pavel Zyrianov
  • 13
  • 4
  • Why are you writing your own user-defined copy, assignment, and move functions? There is no reason to. – PaulMcKenzie Jul 29 '20 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 Jul 29 '20 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 Jul 29 '20 at 17:52
  • The compiler is right, there is no [copy-constructor](https://en.cppreference.com/w/cpp/language/copy_constructor). And as a hint, your move-constructor doesn't move anything, it just copies everything. – Lukas-T Jul 29 '20 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 Jul 29 '20 at 17:56
  • If I not try to do it, how I be able to understand how it all works? – Pavel Zyrianov Jul 29 '20 at 18:04
  • You need to know *when* to write your own. My comment is to show you that it isn't wise to step in and take over the job for no reason, when the compiler does that job correctly and efficiently. When you actually have member variables that require special handling, such as dynamically allocated memory or some resource, then you have a case for writing your own copy-assignment and move operations. – PaulMcKenzie Jul 29 '20 at 18:10
  • Well. I've got a great answers on my question. It would take much more time to obtain them by myself. Now it will be easier for my to go deeper in the subject. Still don't understand why some people so angry with such questions. I'm 48. I'm just trying to get a new profession to live from. And I need more info than just 'Do this. Don't do that. And never ask why.' :) – Pavel Zyrianov Jul 29 '20 at 18:21
  • The issue is really that you haven't done anything that requires thought on how to actually handle cases where it really does matter. Simple types or types where the copy/move is already done for you doesn't actually show you how to properly perform these copy and assignments. Let one of those members be a `char *` that had to call `new[]` to allocate memory instead of `std::string`. If you had that, then the issue of implementing your own copy constructor/move operators become relevant. – PaulMcKenzie Jul 29 '20 at 19:19
  • Also, if an interviewer saw you implementing your own copy operations for the class you've written, that would not be bonus points -- instead that would be points taken off. – PaulMcKenzie Jul 29 '20 at 19:22

2 Answers2

4

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;

    ...
};
Remy Lebeau
  • 555,201
  • 31
  • 458
  • 770
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;
}
Amir Kadyrov
  • 1,253
  • 4
  • 9