0

I have a class with two vectors: an int and an Str. Now I want to define a copy constructor so that the order of elements is reversed; e.g. if a=(1,Hello),(2,World) and I write auto b=a; I get b=(2,world),(1,hello). Which is working perfectly. The issue I am having is overloading the = operator so that the copy constructor is utilized. Here is my Class plus copy constructor:

class grade { private: static int copies; std::vector<int> i; std::vector<std::string> s; public: grade() {copies++;}; grade (int , std::string ); void printval(); void adder(int , std::string ); int getcount(); grade(grade & obj) { std::vector<std::string>::reverse_iterator strIt = obj.s.rbegin(); for (std::vector<int>::reverse_iterator numIt=obj.i.rbegin(); numIt!=obj.i.rend(); ++numIt) { this->adder(*numIt, *strIt); strIt++; } } grade operator =(grade ); };

When I overload the = operator the constructor is called and the trouble is that the valur is not passed to the LHS variable. Here is the overloaded =.

grade grade::operator=(grade cpy) { grade newer = cpy; //Calls the copy constructor as expected return cpy; //No error but blank value is returned to the LHS variable. }

My main function is :

int main() { grade c2(1,"Hello"); grade c1; c2.adder(4,"World"); c2.printval(); std::cout<<std::endl; grade c3 = c2; c3.printval(); std::cout<<std::endl; c1 = c2; std::cout<<std::endl; c1.printval(); return 0; }

Why does c1 remain blank??

Improve this question
1
  • 1
    Never say "works perfectly" in a Stack Overflow question. Commented Feb 6, 2014 at 21:33

4 Answers 4

Reset to default
3

The easiest way to implement the assigment operator is the copy and swap idiom:

grade& operator=( grade other ) { swap( *this , other ); return *this; }

Where swap is a function which takes two grades and swaps its values:

void swap( grade& lhs , grade& rhs ) { using std::swap; swap( lhs.first_member , rhs.first_member ); swap( lhs.second_member , rhs.second_member ); swap( lhs.third_member , rhs.third_member ); ... }

Note that swap() is an overload/specialization of std::swap(). std::swap() uses the assigment operator to perform the swaping by default, so if you use it you lead into infinite recursion. What you have to do is to write your own swap overload which performs member by member swapping (As in the example above).

Note that you should use unqualified swap() to not disable ADL, and let the compiler find the custom swap() overload of the members (If they provide one) or the Standard Library version instead.

Improve this answer
Sign up to request clarification or add additional context in comments.

5 Comments

swap will call operator=, causing infinite recursion. You need to swap the individual member variables. Either that or implement an overload of swap() for your class.
@AdamH.Peterson the idea is you overload swap for your own type.
Ah, I see you've added the overload to your answer. (But you probably should demonstrate the memberwise swap inside your example swap().)
Your use of qualified std::swap will disable ADL, which means if first_member (etc.) implement their own overloads, they will not be considered. You should put using std::swap; in the function and then call swap() unqualified.
@AdamH.Peterson I always forget the ADL issue, thanks
1

You asked: Why does c1 remain blank??

The answer is that the assignment operator does not modify the object itself. Here's your code:

grade grade::operator=(grade cpy) { grade newer=cpy; //Calls the copy constructor as expected return cpy; //No error but blank value is returned to the LHS variable. }

The method returns a copy of the input parameter cpy but does not modify the current instance. When this method is executing, *this is the "LHS variable", and it's not being modified at all. Within this method (operator overload, you need to actually modify the member variables of the current instance!

Improve this answer

7 Comments

I hope that answers your question. For advice on how to actually go about modifying the member variables in the current instance, refer to any one of the other answers that explain the "copy and swap" approach. :-)
The method does not return nothing because its locked at the recursive call to operator=.
@Manu343726 No, this one isn't.
@juanchopanza uups, newer, not *this... Sorry
Well you are right but If I try to modify *this I get an infinite loop.Looks like swap is the only way.
|
1

What you want is the copy-and-swap idiom:

#include <algorithm> grade &grade::operator=( grade cpy ) { using std::swap; swap( i, cpy.i ); swap( s, cpy.s ); return *this; }
Improve this answer

Comments

0

Change your copy constructor and your assignment operator as follows:

grade::grade(const grade& cpy) { *this = cpy; } grade& grade::operator=(const grade& cpy) { if (this == &cpy) return *this; std::vector<std::string>::reverse_iterator strIt = cpy.s.rbegin(); for (std::vector<int>::reverse_iterator numIt=cpy.i.rbegin();numIt!=cpy.i.rend();++numIt) { this->adder(*numIt, *strIt); strIt++; } return *this; }

And as a general scheme:

object::object(const object& input) { *this = input; } object& object::operator=(const object& input) { if (this == &input) return *this; Deallocate(); Allocate(input); Initialize(input); return *this; }
Improve this answer

8 Comments

In my opinion (and I believe in the general C++ community), it is much better to implement assigment in terms of copy (using the copy-and-swap idiom) than to implement copy in terms of assignment.
I have always considered the self assigment check an stupid and unneccesary thing. Consider when you have written a sentence like a = a for last time....
@Manu343726: Possibly. But if the user does something like that, and memory allocation is involved (delete/new), then a runtime error will occur.
@Manu343726: I would tend to agree. Usually if a self-assignment check is necessary for correctness, there is an exception safety problem with the code. If it's an optimization, realize that self-assignment is generally pretty rare, and the self-assignment check will probably not buy you much, or perhaps even cost you.
No, because what you have provided as a general scheme is not a good general scheme at all. The good scheme is to use RAII-based resources, not to manage resources by hand, letting (In almost all cases) the compiler generate a working assigment operator, copy ctor, and dtor (That is, the so named Rule of Zero). In the few cases where RAII aggregation not leads to working default ctors/operator=, is where the copy and swap kicks in.
|