assignment operator template and copy constructor in c++

Question

so basically i m trying to use the assignment operator in way to allocate 2 vars :

S solutionCourante, bestSolution; //(S is a template class)
bestSolution = solutionCourante = solutionInitiale;

Here is the operator i'm dealing with :

template <class S, class T>
const Graphe<S,T> & Graphe<S,T>::operator = (const Graphe<S,T> & graphe)
{

this->lSommets = graphe.lSommets->copieListe(graphe.lSommets);
this->lAretes = graphe.lAretes->copieListe(graphe.lAretes);

return *this;
}

Here is my copy constructor :

template <class S, class T>
Graphe<S,T>::Graphe(const Graphe<S,T> & graphe)
{
 *this = graphe;
}

(I know the constructor copy is a bit bad coded but works)

So at any time, i can see that "bestSolution" and "solutionCourante" are not NULL but empty, and i don't understand why because in my operator "monGraphe" is filled. So it seems like i m doing something wrong when returning the value, first time i m trying to do this operator.

According to :

const Graphe<S,T> & Graphe<S,T>::operator = (const Graphe<S,T> & graphe)

graphe is the item i want to copy and we got *this = graphe?

Hello, english is not my native language sorry, maybe "affectation" is better? — rilent, Feb 25 '15 at 15:43
@rilent, that is called an assignment operator. Please edit your title so it is more accurate. — R Sahu, Feb 25 '15 at 15:46
Why do you use new? You should assign every member of *this, not create a new element. — amchacon, Feb 25 '15 at 15:52
Read up on The Rule of Three. Here's a [starter link](http://en.cppreference.com/w/cpp/language/rule_of_three). — R Sahu, Feb 25 '15 at 15:54
well, i understand the problem now... i deeply thought that "this" was refering to the item i was copying but it's the result. i m editing my post — rilent, Feb 25 '15 at 16:02
@rilent Please see the answer I posted. It uses the code you posted in your question without any changes to the copy constructor. What you needed to do is write the assignment operator correctly, which is done using copy/swap, not with merely using the `=` as your attempt is doing. — PaulMcKenzie, Feb 25 '15 at 16:30

score 2 · Accepted Answer · answered Feb 25 '15 at 16:00

2

An assignment operator is supposed to assign a value to "this", not allocate a new one.

template <class S, class T>
Graphe<S,T> & Graphe<S,T>::operator = (const Graphe<S,T> & graphe)
{
    lSommets = graphe.lSommets ? new PElement<Sommet<T>>(*graphe.lSommets) : nullptr;
    lAretes = graphe.lAretes ? new PElement<Arete<S,T>>(*graphe.lAretes) : nullptr;
    prochaineClef = graphe.prochaineClef;
    return *this;
}
template <class S, class T>
Graphe<S,T>::Graphe(const Graphe<S,T> & graphe)
{
    *this = graphe;
}

Generally speaking, you should not return something allocated on the heap with new, because any ownership information is lost. You should probably try to use smart pointers such as std::unique_ptr.

answered Feb 25 '15 at 16:00

florian

36
2

1

You have memleak with `operator =` as you don't `delete` previous pointer. Once corrected, your constructor will probably invoke UB as `lSommets` and `lAretes` are not initialized. – Jarod42 Feb 25 '15 at 16:05
i ve fixed it by coding 1 method -> copylist() – rilent Feb 25 '15 at 16:07
This answer is topsy-turny as to the way it is usually done. The copy constructor is to do the bulk of the work, while the assignment operator uses copy/swap, using the copy constructor as a "helper". – PaulMcKenzie Feb 25 '15 at 16:08
Sorry this wasn't meant to be an ideal solution, just an answer to the "assignment" misconception. – florian Feb 25 '15 at 16:13

score 1 · Answer 2 · edited May 23 '17 at 10:24

An answer was already posted, but uses a method of having the assignment operator doing most of the work.

Since you already coded the copy constructor, your assignment operator should be written using the copy/swap idiom: What is the copy-and-swap idiom?

What's usually done (if you want synergy between assignment operator and copy constructor) is to have the copy constructor do the bulk of the work, while the assignment operator utilizes the copy constructor (and destructor).

Here is your code using copy/swap:

#include <algorithm>
//...
template <class S, class T>
class Graphe 
{
    //...
    friend void swap(Graphe<S,T>& lhs, Graphe<S,T>& rhs)
    {
        std::swap(lhs.lAretes, rhs.lAretes);
        std::swap(lhs.lSommets, rhs.lSommets);
        std::swap(lhs.prochaineClef, rhs.prochaineClef);
    }
  //...
};
//...
template <class S, class T>
Graphe<S,T>::Graphe(const Graphe<S,T> & graphe) : 
{
    lSommets = graphe.lSommets ? new PElement<Sommet<T>>(*graphe.lSommets) : nullptr;
    lAretes = graphe.lAretes ? new PElement<Arete<S,T>>(*graphe.lAretes) : nullptr;
    prochaineClef = graphe.prochaineClef;
}

template <class S, class T>
Graphe<S,T>& Graphe<S,T>::operator = (Graphe<S,T> graphe)
{
    swap(*this, graphe);
    return *this;
}

A function called swap was added to the template class that merely swaps all of the members between the left and right hand parameter. I emphasize all in the event that you didn't post all of your class members.

Assuming that your copy constructor is bug free, and your destructor is working and is bug-free, the code above will work correctly.

Edit: Made swap a friend function, as suggested by comments from T.C.

i really didnt know about that, thanks for the further infos, i ve tested it and it works nice. So it's more like a coding choice but using the copy/swap method is the true one. — rilent, Feb 25 '15 at 16:55
It is more than a "choice". Writing an assignment operator this way guarantees it will work, provided that you have a working copy constructor and destructor. The link I posted in the answer shows why. — PaulMcKenzie, Feb 25 '15 at 16:56
`swap` should probably be a friend, not a member (or if a member, should swap the argument and `*this`). — T.C., Feb 25 '15 at 17:17

assignment operator template and copy constructor in c++

2 Answers2