= operator overloading

hey,
I am pretty new to C++ and am not sure on this.

Code:

---

SparseMatrix &SparseMatrix::operator=(const SparseMatrix &another)
{
  /*stuff setting up the r,c, and val variables*/
  SparseMatrix retval(r,c,val);
  return retval;
}

---

However when I make an assignment ex:

Code:

---

transpose=test.naiveTranspose();

---

transpose has all null values?
What am I doing wrong?

What are you doing with another?

Code:

---

SparseMatrix &SparseMatrix::operator=(const SparseMatrix &another)
{
  int r[100],c[100],x;
  double val[100];
  for(x=1;x<another.numTerms()+1;x++){
      r[x+1]=another.M[x].i;
      c[x+1]=another.M[x].j;
      val[x+1]=another.M[x].val;
  }
  r[0]=rowDim();
  c[0]=colDim();
  val[0]=numTerms();
  SparseMatrix retval(r,c,val);
  return retval;
}

---

Why all the temporaries in there? Don't you want to copy from another to the current object?

Quote:

---

What am I doing wrong?

---

You are returning a reference to a local variable, for starters.

My suggestion is to implement the copy constructor (you should do that anyway if you want to implement the copy assignment operator), a swap member function, and then implement the copy assignment operator in terms of those two (and the destructor, which you probably also have to write).

Code:

---

SparseMatrix &SparseMatrix::operator=(const SparseMatrix &another)
{
    if (this != &another)
    {
        SparseMatrix temp(another);
        swap(temp);
    }
    return *this;
}

---

Of course, now your problem would have shifted to implementing the copy constructor :)

You only need to implement operator = yourself if the class has member variables that don't on their own already get deep copied (unless there is some intended logic like reference counting intended)
So the first question is, do you even need to write it? To answer this, we need to see the definition of SparseMatrix, in particular the variable declarations inside it.
Then assuming you do need to write it, this should then show us exactly how to write it.

Avoid writing a copy assignment operator that relies on a check for self-assignment in order to work properly; often, that reveals a lack of error safety. If you write the copy assignment operator using the swap idiom (see Item 56), it will automatically be both strongly error-safe and safe for self-assignment

Also: http://www.ubookcase.com/book/Addiso...6lev1sec2.html

Quote:

---

Originally Posted by cpjust

Avoid writing a copy assignment operator that relies on a check for self-assignment in order to work properly; often, that reveals a lack of error safety. If you write the copy assignment operator using the swap idiom (see Item 56), it will automatically be both strongly error-safe and safe for self-assignment

Also: http://www.ubookcase.com/book/Addiso...6lev1sec2.html

---

There's a good possibility laserlight knew that, as it is fine to have that check in there as long as it would also work just fine without it.
http://www.gotw.ca/gotw/023.htm:
4. While you can't rely on the "this != &other" test, there's nothing wrong with using it as an attempt to optimize away known self-assignments. If it works, you've saved yourself an assignment. If it doesn't, of course, your assignment operator should still be written in such a way that it's safe for self-assignment. There are arguments both for and against using this test as an optimization, but that's beyond the scope of this GotW.

Unless you believe self assignment will happen often, it would be more efficient to just swap() without the check for self assignment.

Quote:

---

Unless you believe self assignment will happen often, it would be more efficient to just swap() without the check for self assignment.

---

I agree. On the other hand, I think that a check for self-assignment makes the implementation more robust and clear that we will not choke on self-assignment, the trade-off in efficiency being relatively small anyway.

Quote:

---

Originally Posted by iMalc

You only need to implement operator = yourself if the class has member variables that don't on their own already get deep copied (unless there is some intended logic like reference counting intended)it.

---

To nitpick:Or unless you have non-static const data members.

It probably won't make a noticable difference whether the check is there or not, but I just like to keep things as efficient as possible (without jumping through hoops to get there). A nanosecond saved is a nanosecond earned. :D
I guess if a lot of people on your team aren't quite up to speed on the full power of C++ (which I like to call C+ programmers), then I guess it would be OK to use more basic techniques like checking for self-assignment... But if all the developers on your team are at the same level of expertise, then there's no need to add extra code to spell it out for them.

Quote:

---

Originally Posted by iMalc

4. While you can't rely on the "this != &other" test

---

Why that, even meyers (first edition though) suggest it? is inheritance/polymorphy the reason?

Quote:

---

Originally Posted by King Mir

To nitpick:Or unless you have non-static const data members.

---

Yes, or references - noted.

Quote:

---

Why that, even meyers (first edition though) suggest it? is inheritance/polymorphy the reason?

---

I highly recommend reading the particular GotW and all the rest of them as well, and it should answer your question.

Meh, use Boost's addressof macro instead of & if you're paranoid.

Quote:

---

Originally Posted by iMalc

I highly recommend reading the particular GotW and all the rest of them as well, and it should answer your question.

---

your link is dead. Is the reason inheritance / polymorphy (I mean assignment of derived to base class for example)?

The link merely has a superfluous : at the end. This one works:
http://www.gotw.ca/gotw/023.htm

ah, thx

Here is the variables in my class

Code:

---

/*some methods*/
SparseMatrix &operator=(const SparseMatrix& another);
private:
  class Term;
  int nRows;
  int nCols;
  int nTerms;
  Term* M;
};
.
.
.
/*nested class*/
class SparseMatrix::Term
{
public:
  int i;
  int j;
  double val;
};

---

I understand that it is initializing to 0 because it is a local variable and losing it's memory once the function is complete, but I am not sure on the solution. I believe I have to use all of the temporaries because M is private and thus wouldn't I have to use a constructor with the temp variables instead? M is an array of points that include values ie adjacency matrix.

Since M is a pointer, you need a copy constructor & assignment operator, otherwise the default functions that the compiler creates will only copy the pointer, not the data that it's pointing to.
If the Term class also has pointers or other types that don't do deep copies, you should add copy constructors & assignment operators to all other classes that need them.

I am not sure how to create a copy constructor because the member Term* M is private so I can not learn it's information. Here is what I have. The first constructor is what I already coded and

Code:

---

SparseMatrix::SparseMatrix(SparseMatrix param){
  SparseMatrix retval;
  int rows[100];
  int cols[100];
  double values[100];
/*now I need to obtain param.M[x].i and param.M[x].j and param.M[x].val right?*/
}

---

how do I obtain param.M[x].*?

That's not how a copy constructor works. Look up copy constructors and copy assignment operators in your book and remind yourself of what they are supposed to look like.

Also, it doesn't matter that M is private, since you are in a SparseMatrix function you are allowed to access private member variables of a SparseMatrix object, even if it is a different object than the current one.