6

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;
}
WaelJ
  • 2,942
  • 4
  • 22
  • 28
  • 5
    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`. – dyp Apr 29 '14 at 16:57
  • 3
    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. – Matthieu M. Apr 29 '14 at 17:01
  • 2
    Note: the usage of placement `new` in the assignment operator (in general) was already [dealt with on SO](http://stackoverflow.com/questions/15932333/is-such-assignment-a-good-idea-in-c). In general, it's a very bad idea. Most notably, your class should be marked `final` else it's incorrect. As [Herb Sutter](http://www.gotw.ca/gotw/023.htm) put it so succinctly: *Here Be Dragons*. – Matthieu M. Apr 29 '14 at 17:06
  • @MatthieuM. `B` and `C` form various "chains" of filestream wrappers (e.g. `ZipInputStream`, etc.). They indeed ought to be taking ownership of the pointers. – WaelJ Apr 29 '14 at 21:58
  • @MatthieuM. That is a very good point with deriving classes, though I did not intend this class to be derivable anyways. – WaelJ Apr 29 '14 at 22:00

1 Answers1

4

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;
 }  
Dale Wilson
  • 9,166
  • 3
  • 34
  • 52
  • Aha! And when 'that' gets destructud, it will do the right thing and delete the old pointers in order. – WaelJ Apr 29 '14 at 16:53
  • @WaelJ Only of you use swap instead of assign. – dyp Apr 29 '14 at 16:55
  • 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? – WaelJ Apr 29 '14 at 16:55
  • 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`). – dyp Apr 29 '14 at 17:16
  • 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. – Dale Wilson Apr 29 '14 at 17:25
  • I do, however, agree with your comment that having C point to B this way is dangerous. `shared_ptr` rather than `unique_ptr` might be in order. – Dale Wilson Apr 29 '14 at 17:26
  • 1
    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 :) – dyp Apr 29 '14 at 17:29
  • Thanks, I edited the answer to reflect your and Matthieu's comments. – Dale Wilson Apr 29 '14 at 17:36
  • 1
    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? – WaelJ Apr 29 '14 at 21:55
  • 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. – Dale Wilson Apr 30 '14 at 14:28
  • What if this and that are same one in `ResourceManager& operator=(ResourceManager&& that) `? – heLomaN Jul 28 '22 at 12:53