Memory leak with Memory move assignment operator?

[cjvogl]

I've tracked a memory leak in application code down to the move assignment operator in the Memory class.

I effectively have a function like

Vector doStuff(const Vector &input)
{
  Vector output;
  // do stuff
  return output;
}

If my function is called with an assignment to an uninitialized vector like

Vector a({1,2,3,4});
Vector b = doStuff(a);

things are all great. If my function is called to an initialized vector like

Vector a({1,2,3,4});
Vector b(5);
b = doStuff(a);

the 5 original doubles for b become a memory leak because the corresponding move assignment operator for Vector is

Vector &Vector::operator=(Vector &&v)
{
   data = std::move(v.data);
   // Self-assignment-safe way to move v.size to size:
   const auto size_tmp = v.size;
   v.size = 0;
   size = size_tmp;
   return *this;
}

and the move assignment operator for Memory is

Memory &operator=(Memory &&orig)
{
   // Guard self-assignment:
   if (this == &orig) { return *this; }
   *this = orig;
   orig.Reset();
   return *this;
}

While I could modify my function signature to void doStuff(const Vector &input, Vector &output), I can see others wanting to use something like the original function signature to leverage the move assignment operator for Vector. My naive fix for this is to add a line to the move assignment operator for Memory to delete the old data, i.e.,

Memory &operator=(Memory &&orig)
{
   // Guard self-assignment:
   if (this == &orig) { return *this; }
   Delete();
   *this = orig;
   orig.Reset();
   return *this;
}

but I can imagine this may cause some issues elsewhere?

[pazner]

Yeah, this looks like a bug in the move assignment operator of Vector.

I don't think Memory should delete on move (the semantics of Memory are like those of a pointer).

Perhaps we can just use Vector::Swap to implement move assignment. I think that should fix this issue.

[cjvogl]

@pazner, thanks for the quick response!

I don't think Memory should delete on move (the semantics of Memory are like those of a pointer).

That makes sense.

Perhaps we can just use Vector::Swap to implement move assignment. I think that should fix this issue.

This also makes sense: I can implement that and test it out with the application code I'm using (unless you wanted to tackle it on your own).

[pazner]

Can you see if #4500 fixes the issue for you?

[cjvogl]

Works greats, thanks @pazner!

As a general musing and perhaps note to myself, I suppose a Memory object could maintain a counter to the associated data so that the data can be deleted once there are no more Memory objects/pointers associated with the data (something like std::shared_ptr).