0

I have some doubts about copy constructor and assignment operator. I know that when I define a constructor, the default constructor is not synthetized by the compiler. My doubt is if it is ok to define just the copy constructor. I would say no since if I define the copy constructor the default constructor is not synthetized so I cannot initialize objects, because I would need an object of that class, which I don't have. I don't know if this is right. My second doubt is about value-like implementation of a class containing a pointer. Every code I've seen until now use the new operator inside both copy and assignment operator. For instance:

#include <string>
#include <vector>
#include "book.hh"

class Student
{
    std::string name;
    unsigned int id;
    unsigned int age;
    char gender;
    std::vector<Book> * books;

 /*Copy-constructor*/
    Student (const Student & other)
    {
        name = other.name;
        id = other.id;
        age = other.age;
        gender = other.gender;
        books = new std::vector<Book> (*other.books);
    }

 /*Assignment operator*/
    Student & operator = (const Student & other)
    {
        if (this != &other)
        {
            name = other.name;
            id = other.id;
            age = other.age;
            gender = other.gender;
            delete books;
            books = new std::vector<book> (*other.books);
        }
        return *this;
    }
 }

The document says that a constructor should be implemented. What bout the constructor? How can I instantiate a class without having a constructor(which is not the copy constructor), in this case? Morover, I don't understand why it uses new in the copy constructor and in the assignment operator. I would do for instance, in the assignment operator body, *books = *(other.books); is this also correct?

Peanojr
  • 185
  • 1
  • 8
  • `books` needs a memory slot for `*books = *(other.books);` to work. Thus, you need to allocate memory for your vector. Of course the use of a dynamically allocated vector is questionable, but I'm not sure your question is here – Adrien Givry Jul 08 '19 at 19:23
  • 1
    ... but since you have a `vector` to hold `Book`s - `new` seems like a bad idea. – Ted Lyngmo Jul 08 '19 at 19:24
  • 2
    Why pointer of vector? – Kevin Kouketsu Jul 08 '19 at 19:24
  • Do: `std::vector books;` and in the copy ctor and copy assignment operator: `books = other.books;` – Ted Lyngmo Jul 08 '19 at 19:25
  • 3
    change `std::vector * books;` to `std::vector books;` and you don't need to define any of the special member functions. – NathanOliver Jul 08 '19 at 19:25
  • ... what @NathanOliver said :-) However, If you do define those methods, you can just copy `books` like I mentioned above. – Ted Lyngmo Jul 08 '19 at 19:26
  • I don't need to modify the code, I haven't written this. I just want to understand how value like implementation works, why operator new is used in the copy constructor and in the assignment operator, and if it is correct not to define another constructor when I've defined a copy constructor like in this case. – Peanojr Jul 08 '19 at 19:40
  • @AdrienGivry No my question is exactly that, why do I have to use dynamic memory; when I call assignment operator a pointer already exists, why should I not change just the value to which it points? – Peanojr Jul 08 '19 at 19:44
  • The assignment operator has a flaw in that it alters the members of the object before attempting to allocate the new vector. What if `new` throws an exception? You have deleted the memory, and thus corrupted the object. – PaulMcKenzie Jul 08 '19 at 19:47
  • @Peanojr If you just change the pointer value (`books = other.books;`) you'll end-up having 2 instances of Student pointing to the same memory slot. Meaning that when one of the two students will get destructed, the memory for these books will be freed, and the other student will have a dangling pointer. – Adrien Givry Jul 08 '19 at 19:48
  • The big point is `new` should NOT be needed here at all. This is "bad" code. But since you're stuck with it, `std::vector * books;` defines a pointer to a `std::vector`. When you construct a `Student` a `std::vector` must be provided. You could simply assign `books = other.books`, but then you have two students pointing to the same books. Add a `book` to one `Student`, it's added to the other `Student`. If one `Student` is destroyed and a destructor is provided to `delete books;`, BOTH `Student`s lose their `books`, this is probably fatal. – user4581301 Jul 08 '19 at 19:48
  • Explanation of why this is "bad" code: The entire point of `vector` is to manage a list of items for you. It handles adding, removal, copying, moving, resizing, construction, destruction, and everything else you could want to do to a list. By having a pointer to one, you now must manage the copying, moving, construction, and destruction, and these can be non-trivial tasks. – user4581301 Jul 08 '19 at 19:52
  • @Peanojr *Every code I've seen until now use the new operator inside both copy and assignment operator* -- Then you didn't look at code that uses copy/swap: `Student& operator=(const Student & other){ if (&other != this) { Student temp(other); std::swap(name, temp.name); std::swap(id, temp.id); std::swap(age, temp.age); std::swap(gender, temp.gender); std::swap(books, temp.books);} return *this); }` -- That does not use `new` in the assignment operator, plus it removes the flaw that I mentioned previously. – PaulMcKenzie Jul 09 '19 at 02:43

1 Answers1

0

Check out The Rule of Three, which states that you should typically define a copy constructor, an assignment operator, and a destructor if any are needed. If you have a reason to define one of these, you almost certainly have a reason for defining the others. You are managing dynamic memory, which means you should likely initialize that in the constructor.

Your copy and assignment operators have to do new std::vector<Book> (*other.books); because if they only copy the pointers, like books = other.books;, you will end up with two Students sharing the same Books, which is (usually) a recipe for disaster.

(Side note: you can save some headache with the copy-and-swap idiom.)

Finally, make sure your destructor deletes any memory you allocate. And as @Ted Lyngmo noted, in this specific case, using a plain std::vector instead of a pointer to one will eliminate the need to define any of the special member functions.

joshwilsonvu
  • 2,569
  • 9
  • 20
  • Rule of three is about the copy ctor, the assignment op, and the destructor - nothing about the default ctor. In fact, most classes will probably not provide a default cto - the OP's class should probably not provide one. –  Jul 08 '19 at 19:38
  • Fixed, and you're right but OP seems to want to understand copy/assignment in general. – joshwilsonvu Jul 08 '19 at 19:44
  • 2
    Better side note : there's no reason to `new` an `std::vector` in the first place. – François Andrieux Jul 08 '19 at 19:54
  • Thank you for the answer, I know the rule of three. My doubt is why I use dynamic memory. I mean, I just have a pointer in my class, this does not imply I have to store it in a dynamically allocated object. My question is about the choice of use dynamic memory. I would not do books=other.books, but *books=*other.books. In this code there is not even the constructor, and I don't understand if there should be. – Peanojr Jul 08 '19 at 19:54
  • 1
    `*books = *other.books` is "assign the content at the location pointed to by `other.books` to the location pointed to by `books`". This will put you in a world of hurt, because what does `books` point to? Nothing! It hasn't been initialized by a default constructor. You can't assign to memory you don't own. – joshwilsonvu Jul 08 '19 at 20:02
  • 1
    @Peanojr A pointer that does not point at a valid object is almost useless (One exception is as a sentry in a graph). It does not have to point to a a dynamically allocated object, but the alternatives are not particularly appealing here. An Automatic allocation will go out of scope and leave the [pointer dangling](https://en.wikipedia.org/wiki/Dangling_pointer). Pointing to a global `books` is bad (everybody is pointing to the same thing), pulling a `books` out of a list of free `books`'s runs the risk of running out of free `books`'s. The best solution is to not use a pointer here at all. – user4581301 Jul 08 '19 at 20:02
  • *You can't assign to memory you don't own.* Unfortunately you can. Sometimes it can look like you got away with it, but this is actually the WORST thing that can happen. Somewhere, and you don't know where, memory got corrupted. You won't know this until the program exhibits bad behaviour, and it will almost certainly happen somewhere that the bug isn't. It's quite possibly the nastiest bug in the world to find and solve. – user4581301 Jul 08 '19 at 20:10
  • @user4581301 So from what I understand the problem of using *books=*other.books is that books has not been initialized. But is this code right? I think there should be a constructor( not only the copy constructor) or I can't create objects. If I define the default constructor the pointer should be defaulted initialized, in this last case there is stil the problem of using *books=*other.books? – Peanojr Jul 08 '19 at 20:16
  • 1
    The default initialization of a pointer is nondeterministic junk, so yes, there is still the problem of assigning to that location. It must point at a valid `std::vector` before anything can be assigned. – joshwilsonvu Jul 08 '19 at 20:18
  • @JoshWilson this answers half of my question, and motivates the use of new. There is still the problem that there is not the constructor in this code. Noone told anything about that, so I don't knwow. Can I create object of this class, with the given code. I think I would get error since I don't have a constructor but I have the copy constructor, but I'm not sure. – Peanojr Jul 08 '19 at 20:22
  • I think no one's answered the bit about the default constructor for you because it's really easy to look up or test. [For example.](https://ideone.com/dbVtoo). – user4581301 Jul 08 '19 at 20:33
  • *If I define the default constructor the pointer should be defaulted initialized, in this last case there is stil the problem of using *books=*other.books?* What happens in one constructor does not happen in another unless you ask for it to happen with a [Delegating Constructor](https://en.cppreference.com/w/cpp/language/initializer_list#Delegating_constructor). So one way or another you need to allocate in all of your constructors. One alternative (that doesnt fir this use case is [Two Phase Construction](https://stackoverflow.com/questions/3021572/two-phase-construction-in-c). – user4581301 Jul 08 '19 at 21:46