Is it safe to use placement new on 'this' pointer

Question

Current Implementation

I have a class containing unique_ptr fields which depend on one other:

class ResourceManager {
  ResourceManager() {}

  ResourceManager(A* a_ptr) :
    b_ptr(new B(a)),
    c_ptr(new C(b_ptr.get())) {}

  ResourceManager& operator=(ResourceManager&& that) {
    // Call destructor, then construct a new instance on top
    ~ResourceManager();
    ResourceManager* new_this = new(this) ResourceManager();

    // Surely this must be the case, right?
    // Is there any reason to prefer using either?
    assert(new_this == this);

    new_this->b_ptr = that.b_ptr;
    new_this->c_ptr = that.c_ptr;

    return *new_this;
  }

  unique_ptr<B> b;
  unique_ptr<C> c;
};

Use case

The use case here is that I would like to reassign new values to the pointers, whilst keeping the ResourceManager as a stack-allocated variable, or as a non-pointer class member.

With my current setup I imagine using it something like this:

A a, another_a;
ResourceManager r(&a);

// Use r...

// Destroy old ResourceManager and create the new one in place.
r = ResourceManager(&another_a);

The reason this is even a problem is due to the fact that B and C are non-assignable (for e.g. file streams)

Ugly Alternative

An alternative uglier (and dangerous) method would be to explicitly reset the unique_ptr fields crucially in reverse order (remember that C depends on B, and hence must be destructed first), effectively mimicking the default destruction behaviour.

ResourceManager& operator=(ResourceManager&& that) {
  // Mimic destructor call (reverse-order destruction)
  c_ptr.reset();
  b_ptr.reset();

  b_ptr = that.b_ptr;
  c_ptr = that.c_ptr;

  return *this;    
}

Note that a wrong implementation would be to simply use the default assignment operator for ResourceManager. This will assign the field in-order which implies in-order destruction of the unique_ptrs, whereas we require reverse-order destruction.

Questions

Is this usage of this pointer with placement new and the explicit destructor call safe?

Must I use the returned new_this pointer as opposed to the original this pointer (for example, if the this pointer technically becomes invalidated after calling the destructor)?

Are there any better suggested ways to achieve this? If add more such unique_ptr fields to the class, I would have to make sure that I add a copy to the assignment operator. For instance, is it possible to call the move constructor instead, like so:

ResourceManager& operator=(ResourceManager&& that) {
  // Call destructor
  ~ResourceManager();

  // Move-construct a new instance on top
  ResourceManager* new_this = new(this) ResourceManager(that);
  return *new_this;
}

Orderly

Typically, the simplest way to implement an assignment operator is the copy-and-swap technique; or its move-and-swap equivalent: Take ResourceManager by value and then swap this parameter with *this. – Arda

Why is C depending on an instance of B that it does not own ? This is a recipe for failure and difficulties. If you could solve this root issue, then you would not have to worry about the defaulted assignment operator doing the wrong thing. – Plenipotentiary

Note: the usage of placement new in the assignment operator (in general) was already dealt with on SO. In general, it's a very bad idea. Most notably, your class should be marked final else it's incorrect. As Herb Sutter put it so succinctly: Here Be Dragons. – Plenipotentiary

@MatthieuM. B and C form various "chains" of filestream wrappers (e.g. ZipInputStream, etc.). They indeed ought to be taking ownership of the pointers. – Orderly

@MatthieuM. That is a very good point with deriving classes, though I did not intend this class to be derivable anyways. – Orderly

Answer 1

Your solution seems overly complex.

I would code it like this:

class ResourceManager {
  ResourceManager() {}

  ResourceManager(A* a_ptr) :
    b_ptr(new B(a)),
    c_ptr(new C(b_ptr.get())) {}

  ResourceManager& operator=(ResourceManager&& that) 
  {
    // the order of these moves/assignments is important
    // The old value of *(this->c_ptr) will be destroyed before
    // the old value of *(this->b_ptr) which is good because *c_ptr presumably
    // has an unprotected pointer to *b_ptr.
    c_ptr = std::move(that.c_ptr);
    b_ptr = std::move(that.b_ptr);
    //  (a better solution might be to use shared_ptr<B> rather than unique_ptr<B> 
    return *this;
  }

  unique_ptr<B> b_ptr;
  unique_ptr<C> c_ptr;
};

Note: When the move assignment returns, that will "empty" meaning both that.b_ptr and that.c_ptr are nullptr. This is the expected result of a move assignment.

Or if "reconstructing" the target of the assignment is important (assuming there's extra code not shown in this example that makes it so) I might add a move constructor and a swap method like so:

 ResourceManager(ResourceManager&& that)
 : b_ptr(std::move(that.b_ptr)),
   c_ptr(std::move(that.c_ptr))    
 {
 }

 void swap(ResourceManager & that)
 {
   b_ptr.swap(that.b_ptr);
   c_ptr.swap(that.c_ptr);  
 }

 ResourceManager& operator=(ResourceManager&& that) 
 {
     ResourceManager temp(std::move(that));
     this->swap(temp);
     return *this;
 }  

Trifoliate

Aha! And when 'that' gets destructud, it will do the right thing and delete the old pointers in order. – Orderly

@Orderly Only of you use swap instead of assign. – Arda

Could we also not simply do: r = ResourceManager=std::move(ResourceManager(new_a)); and so it would not be necessary to implement an assignment operator at all? – Orderly

On assignment. b_ptr = std::move(that.b_ptr); destroys what b_ptr points to, lets b_ptr point to what that.b_ptr points to and then empties that.b_ptr (sets the pointer to nullptr). – Arda

True that gets destroyed, but that is expected (I edited my answer to mention this.) but you still have *this pointing to a B and a C with the C pointing to the corresponding B. – Trifoliate

I do, however, agree with your comment that having C point to B this way is dangerous. shared_ptr<B> rather than unique_ptr<B> might be in order. – Trifoliate

That was Matthieu's comment ;) -- anyway, the problem is not that, but *this. As long as *(this->c_ptr) contains a reference to *(this->b_ptr), the former should be destroyed before the latter. So you should assign to those pointers in reverse order (first to this->c_ptr, then to this->b_ptr) so that the objects currently owned by this->X_ptr get destroyed in the right order. Or use move-and-swap :) – Arda

Thanks, I edited the answer to reflect your and Matthieu's comments. – Trifoliate

Thanks for the feedback guys! However, is there any reason why I cannot use std::swap directly rather than defining swap as you did above? – Orderly

std::swap won't work because it requires copyable parameters. The compiler won't generate a copy constructor for ResourceManager, and you can't write one yourself because that would require copying unique_ptrs Try it. It's a good idea to recognize the error messages that result since they are not obvious. – Trifoliate

What if this and that are same one in ResourceManager& operator=(ResourceManager&& that) ? – Phelan