c++ operator = working weird (output ok inside the function, but wrong after return..)

Question

I have a program that I've been modifying to extract some data.
Recently I did another data extraction and used a work-around for lack of time.
Now, I am trying to find out what the problem was, because I have some spare time.

Below, there is a function Act() which is a member function of class Mat, and it forms a new matrix with altered data values. The Mat has a pointer to a buffer containing actual data. I found that after the Act() function which returns a Matrix, the 'operator =' function is called. (there was no 'operator =' function so I added it. maybe default '= operator' was being used. With or without my added 'operator =' function, it does not produce the final right output) Here is the portion of the '=' operator which is returning the data(type Mat).(it is a c++ code but I just used printf because it works anyway)

template<typename T>
Mat<T> Mat<T>::operator = (const Mat<T>& rhs)
{
  Mat<T> result(rhs.row_, rhs.col_);
  int num_bytes = sizeof(T) * rhs.row_ * rhs.col_;
  printf("ope = , res val = %x, src val = %x, row = %d, col = %d\n", result.val_, rhs.val_, rhs.row_, rhs.col_);
  for(int i=0;i<10;i++){
    printf("%04x ",((half_float::half *)rhs.val_)[i].data_);
    }
  printf("\n");
  memcpy(result.val_, rhs.val_, num_bytes);
  for(int i=0;i<10;i++){
    printf("%04x ",((half_float::half *)result.val_)[i].data_);
    }
  printf("\n");
  return result;
}

ope = , res val = 12199b0, src val = c07680, row = 128, col = 128
3be0 bbfc bbf5 3bf8 3af0 bbf6 bbef b29f bbaf 3bfd
3be0 bbfc bbf5 3bf8 3af0 bbf6 bbef b29f bbaf 3bfd

This '3be0 bbfc bbf5 ...' is the right pattern.

The calling part looks like this.

  printf("calling activation..\n");
  d0_out = d0_out.Act(NN_CELL::AT_TANH);
  std::cout << "MLP Layer 0 executed." << std::endl;
  for(int i=0;i<10;i++){
    printf("%04x ",((half_float::half *)d0_out.val_)[i].data_);
    }
  printf("\n");

But after the assignment, seen from the main function, the data is weird(inside the = operator, the copy seems to have been made correctly, but outside, it's doesn't).

MLP Layer 0 executed.
40de c2e2 c1eb 425e 3d4a c21b c187 b2b8 bfce 4358

As you can see, the data is wrong.
The only way I can produce right output is using a new object and assigning the result of Act() function to it.

NN_CELL::Mat<T> d0_out1 = d0_out.Act(NN_CELL::AT_TANH); // tmp work-around

Then, d0_out1 (new object) contains the right data(3be0 bbfc bbf5 ..), instead of original

d0_out = d0_out.Act(NN_CELL::AT_TANH);

What is wrong with the original code?

Assignment is supposed to modify `*this`, not create a new object. The code that produces the correct output is not an assignment but an initialisation. (And you need to read about "the rule of three".) All of this is covered in [decent books](https://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list). — molbdnilo, Jan 23 '19 at 07:47
... which makes me think this is an implementation with the copy-and-swap idiom that forgot about the "swap". — StoryTeller - Unslander Monica, Jan 23 '19 at 07:48
Also, consider using copy-and-swap idiom instead of handwritten `operator=` — bartop, Jan 23 '19 at 07:59
@molbdnilo maybe I'm not aware of something important. I want to keep the Act() function as is(to return a new object), then can't I do 'a = a.Act()' to replace a? and Why was the '= operator' called? — Chan Kim, Jan 23 '19 at 08:06
@ChanKim I'm not sure what you're wondering. If you have overloaded the operator, `a = a.Act()` gets translated to `a.operator=(a.Act())`. (Note that the return value is unused.) — molbdnilo, Jan 23 '19 at 08:32

score 1 · Answer 1 · answered Jan 23 '19 at 07:53

operator= should modify *this, not create a new temporal object X, copy the received data to X and then return X. You have created a temporal object but the data of the class that runs operator= has not been modified!

Try this code:

template<typename T>
Mat<T> Mat<T>::operator = (const Mat<T>& rhs)
{
  int num_bytes = sizeof(T) * rhs.row_ * rhs.col_;
  printf("ope = , res val = %x, src val = %x, row = %d, col = %d\n", this->val_, rhs.val_, rhs.row_, rhs.col_);
  for(int i=0;i<10;i++){
    printf("%04x ",((half_float::half *)rhs.val_)[i].data_);
    }
  printf("\n");
  memcpy(this->val_, rhs.val_, num_bytes);
  for(int i=0;i<10;i++){
    printf("%04x ",((half_float::half *)this->val_)[i].data_);
    }
  printf("\n");
  return *this;
}

thanks! this is also the answer.But I saw the other one and chose it first, sorry. (and the other one gave me some other explanation). :) — Chan Kim, Jan 23 '19 at 08:47

VLL · Accepted Answer · 2019-01-23T08:15:15.303

a = a.Act() does not work, because you have implemented operator= incorrectly. The default operator= does not work in this case because you have a pointer in your class, and when the temporary returned by Act() is destroyed, it probably invalidates the pointer.

operator= is called, because you wrote = in your code. When you use "tmp work-around", you create a new object, which does not call operator= but the copy constructor. This seems to work.

As for how to implement operator= correctly: Assignment operator should modify the existing object, instead of returning a new one.

For example:

struct A {
    int row_, col_;
    float *val_;

    A& operator=(const A& rhs) {
        if (this != &rhs) {
            const int num_bytes = sizeof(float) * rhs.row_ * rhs.col_;
            row_ = rhs.row_;
            col_ = rhs.col_;
            memcpy(val_, rhs.val_, num_bytes);
        }
        return *this;
    }
};

Wow, that works. I didn't know `operator=` should return `*this`. :). and I realize if I use 'a = ', the `operator=` for `a` is used and I need to implement the `deep copy` there. Thanks tons! — Chan Kim, Jan 23 '19 at 08:37

score 1 · Answer 3 · answered Jan 23 '19 at 08:00

As the comments state correctly, the Assignment Operator is intended to modify an already existing object. You're creating a new one and returning that.

Try something like this:

template<typename T>
Mat<T>& Mat<T>::operator = (const Mat<T>& rhs)
{
  row_ = rhs.row_;
  col_ = rhs.col_;
  delete val_;
  val_ = new T[row_*col_];

  int num_bytes = sizeof(T) * rhs.row_ * rhs.col_;
  printf("ope = , res val = %x, src val = %x, row = %d, col = %d\n", val_, rhs.val_, rhs.row_, rhs.col_);
  for(int i=0;i<10;i++){
    printf("%04x ",((half_float::half *)rhs.val_)[i].data_);
    }
  printf("\n");
  memcpy(val_, rhs.val_, num_bytes);
  for(int i=0;i<10;i++){
    printf("%04x ",((half_float::half *)val_)[i].data_);
    }
  printf("\n");
  return *this;
}

c++ operator = working weird (output ok inside the function, but wrong after return..)

3 Answers3