Copy assignment definition

[frek]

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

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! :|

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:

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?

[dutch]

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.

[frek]

if (this != &a) { } around it

I agree with this:

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.

[keskiverto]

Pointers are harmless. Operations that could throw, are not.

[frek]

@keskiverto
Would you write that assignment operator definition in my last post above differently, please?

[malibor]

@frek you last example is wrong:
else return;

you must return a reference to this object not returning void

[frek]

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?

[MikeyBoy]

Not true. If your function specifies a return type, you must return something of that type.

Surely your compiler was warning you about this?

[keskiverto]

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.

[dutch]

frek wrote:

This is my opinion.

So what?
You're a beginner who doesn't know what he's talking about.
Your opinion is totally worthless.

[deleted account xyzzy]

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.

[malibor]

@frek
Your assignment operator should look like this:

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;
}

[frek]

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

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;
} 

[frek]

@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.

[malibor]

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.

[keskiverto]

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:

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.

[frek]

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:

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!

[malibor]

@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.

[frek]

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?

[malibor]

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.