Copy assignment definition

Pages: 123
Hello all,
Please take a look at this class, Vector:

 ``123456789101112131415`` ``````class Vector { private: double* elem; // elem points to an array of sz doubles int sz; public: Vector(int s); // constructor: establish invariant, acquire resources ~Vector() { delete[] elem; } // destructor: release resources Vector(const Vector& a); // copy constructor Vector& operator=(const Vector& a); // copy assignment double& operator[](int i); const double& operator[](int i) const; int size() const; };``````

Stroustrup, in one of his books, has defined it this way, but my bad, I don't like it! :|

 ``12345678910`` ``````Vector& Vector::operator=(const Vector& a) // copy assignment { double* p = new double[a.sz]; for (int i=0; i!=a.sz; ++i) p[i] = a.elem[i]; delete[] elem; // delete old elements elem = p; sz = a.sz; return *this; }``````

The version I like is:

 ``123456789`` ``````Vector& Vector::operator=(const Vector& a) // copy assignment { delete[] elem; // delete old elements elem = new double[a.sz]; sz = a.sz; for (int i=0; i!=a.sz; ++i) elem[i] = a.elem[i]; return *this; } ``````

My reason:
I don't see any clue why we should define a pointer (`p`) as a proxy to copy a vector into another!
Furthermore, we "may" have memory leak because `p` is not deleted, even if the assignment to `elem` makes another call to the copy constructor function. And lastly, the later version is simpler, cleaner and more straightforward.
To what extent do you agree, please?

Last edited on
Your implementation is broken. You need to add if (this != &a) { } around it. Stroustrup's works with self-assignment, although in perhaps an inefficient way.

There's no potential memory leak in Stroustrup's version. Assigning a pointer to another pointer doesn't copy anything except the pointer itself.

And if new throws, elem will still point to the old memory in Stroustrup's version.
Last edited on
 if (this != &a) { } around it
I agree with this:
 ``123456789101112`` ``````Vector& Vector::operator=(const Vector& a) // copy assignment { if (this != &a) { delete[] elem; // delete old elements elem = new double[a.sz]; sz = a.sz; for (int i=0; i!=a.sz; ++i) elem[i] = a.elem[i]; return *this; } else return; } ``````

 There's no potential memory leak in Stroustrup's version
But then we will have two pointers `p` & `elem` pointing to the same address. We need `elem` but not `p`. This is my opinion.
Last edited on
Pointers are harmless. Operations that could throw, are not.
@keskiverto
Would you write that assignment operator definition in my last post above differently, please?
@frek you last example is wrong:
`else return;`

you must return a reference to `this` object not returning `void`
It return nothing and the control goes out of the function to the assignment. So nothing happens which what we expect when we assign an object to itself. Not true?
Not true. If your function specifies a return type, you must return something of that type.

 It return nothing and the control goes out of the function to the assignment. So nothing happens which what we expect when we assign an object to itself. Not true?

Not true. If you promise to return a value, then you must return a value.

Consider this:
`b = a = a;`
If `a = a` returns nothing, then what is assigned to b?

The other thing; I trust Stroustrup.
frek wrote:
This is my opinion.

So what?
You're a beginner who doesn't know what he's talking about.
 To what extent do you agree, please?

You've shown you really don't want to hear/read this, but you asked. But really, do prove me wrong. I won't be offended.

AND you prove you are not willing to take criticism or advice by being a piss-ant report monkey.

Disagree totally. Stroustrup knows more about C++ than most C++ programmers. Even long-time C++ programmers. You liking a different way to do things is not really up for debate, unless you can convince the ISO committee your way is:

1. better.
2. less error prone.
3. doable without breaking other parts of C++.

There are definite parts of C++ that I don't like and IMO could be done better. But from my admittedly limited experience deviating from PROVEN practices for trying to write bullet-proof code can be risky and likely to make code buggy.

std::string. Not the most elegant interface in a few particulars, the designers could have done things differently. But that interface every programmer knows how to deal with. Changing the interface could break a LOT of pre-existing code.

http://www.cplusplus.com/forum/lounge/270245/#msg1164366

Smart pointers is another area IMO with lvalue/rvalue/move semantics that could be "improved."

Exactly how? I have vague ideas how, but the ISO committee made their choices for reasons I don't know about. And honestly would not understand if I was told why.

+-----+

Regarding your function: you wrote a contract the function would return a value. If you do not return a value every time you violate that contract.

Returning nothing (no explicit return statement) is not part of the contract you wrote and agreed to abide by. C++ is filing a lawsuit -- warning/error message(s) -- for failure to uphold the contract you made.
Last edited on
@frek
Your assignment operator should look like this:

 ``123456789101112131415161718192021`` ``````Vector& Vector::operator=(const Vector& a) // copy assignment { if (this != &a) { // create new storage double* p = new double[a.sz]; // copy elements from a for (int i=0; i!=a.sz; ++i) p[i] = a.elem[i]; delete[] elem; // delete old elements // do assignment elem = p; sz = a.sz; } // if "a" is "this" object, there is nothing to do return *this; }``````

@keskiverto, @MikeyBoy. Right, thanks.
So I use this version when needed.

 ``1234567891011`` ``````Vector& Vector::operator=(const Vector& a) // copy assignment { if (this != &a) { delete[] elem; // delete old elements elem = new double[a.sz]; sz = a.sz; for (int i=0; i!=a.sz; ++i) elem[i] = a.elem[i]; } return *this; } ``````

@malibor

Oh, I got it.
If we delete old elements `delete[] elem;` then we can't assign to it later on `elem = new double[a.sz];`, hence we need that pointer `p`.
Last edited on
Yes you can, becase `elem` has been assigned with new memory with:
`elem = p;`

pointer `p` went out of scope and only the pointer was destroyed not the memory at which it pointed, this memory was now assigned to `elem` and pointer `elem` is a valid pointer until you delete memory at which it points.

once you `delete[]` it, you need to assign it to new memory with `elem = new double[]`

and then it's again valid valid pointer.

If you just do `elem = new double[]` without `delete[] elem` first then you leak memory previously pointed by `elem`.
Last edited on
 If we delete old elements delete[] elem; then we can't assign to it later on elem = new double[a.sz];

Wrong.

The `delete[] elem;` does not modify the `elem`. Not at all.
The `delete[]` deallocates a block of memory in Free Store.
The `delete[]` reads the address of that block from variable `elem`.

The `elem = new double[a.sz];` does not care about the value of `elem`.
The `new` allocates block of memory from Free Store and writes the address of that block into variable `elem`.

Consider this:
 ``12`` ``````int x = 7; x = 42; // horror, can we write a value to x, for it has a value already? ``````

Yes, we can.
Yes, we can write a value to elem too.
Thanks. Yeah right.

So, probably as the last point in this case, please tell me what the problem is with my prior code and why still we need that pointer `p` in it:

 ``1234567891011`` ``````Vector& Vector::operator=(const Vector& a) // copy assignment { if (this != &a) { delete[] elem; // delete old elements (free the allocated storage) elem = new double[a.sz]; sz = a.sz; for (int i=0; i!=a.sz; ++i) elem[i] = a.elem[i]; } return *this; } ``````

PS: someone is reporting my posts regardless of the thing I write into them! Funny!
@frek
Somebody is reporting you because you try to be smarter than B. Stroustrup.

Your last 2 sample codes also try to make Stroustroup's sample "faster".
You are missing the meaning of temporary pointer `p`, to you it's just a nonsense which slows my program down right?

Even though I highly doubt he would write the code you posted in your initial post.
Funnier, indeed! Hhhhhh
If trying to be smarter than Stroustrup is a problem, I always try to be smarter than him.
And you must completely be sure that I like him more than you do! Let's bow out of these untechnical discussions and reach a reason. I need a reason.

I'm not sure I understood your reason above properly. Did you mean that using a temporary pointer (here P) makes the program run faster, please!? If so, why?

Last edited on
What do you return from operator if `elem = new double[a.sz];` throws?
but you deleted `elem` without assigning it to `nullptr`, so its now a dangling pointer.

Ofc all of this depends on how exception handling is done etc. in this simplistic example this probably won't happen, but it's not bad practice to write safe code even if it's just about showing an example.
Pages: 123