Using std::unique_ptr and std::move

Question

I'm trying out new things (on class Inproc), specifically using std::unique_ptr and std::move, and I'm wondering if I'm doing it right.

The code compiles so that gives me hope. Still, what do you think?

Note: I'm working without exceptions for reasons I won't go into. That means the factory functions are required to handle errors. Keep that in mind please.

class Fan {
public:
 static Fan * create(Fan *peer = NULL) { return new Fan(peer); }
 virtual ~Fan();
private:
 Fan(Fan *peer__ = NULL) : peer_(peer__) { };
 Fan *peer_;
};
class Inproc {
public:
 static Inproc * create();
 ~Inproc();
private:
 Inproc(std::unique_ptr<Fan> bound__,
 std::unique_ptr<Fan> connected__);
 std::unique_ptr<Fan> bound_;
 std::unique_ptr<Fan> connected_;
};

---

Inproc * Inproc::create() {
 std::unique_ptr<Fan> bound(Fan::create());
 if (bound) {
 std::unique_ptr<Fan> connected(Fan::create(bound.get()));
 if (connected) {
 return new(std::nothrow) Inproc(std::move(bound), std::move(connected));
 }
 }
 return NULL;
}
Inproc::Inproc(std::unique_ptr<Fan> bound__,
 std::unique_ptr<Fan> connected__)
 : bound_(std::move(bound__)), connected_(std::move(connected__)) { }

• Goswin von Brederlow: Would it be possible to have Inproc(Fan &&bound__, Fan &&connected__); as constructor and still use std::unique_ptr and std::move?
• nwp: The code is correct as far as move semantics go. I would like you to say what Inproc and Fan are actually supposed to do before reviewing.
• Goswin von Brederlow: @nwp: Fan does fair queueing of incoming messages and round-robin on outgoing messages and Inproc connects 2 fans for in process communication (as opposed to network).
• Goswin von Brederlow: I'm unsure about having to use std::move() twice. But that is required if Inproc() takes std::unique_ptr<Fan> or std::unique_ptr<Fan>&&. On the other hand if Inproc() takes std::unique_ptr<Fan>& then the std::move in create becomes uneccessary. Is that a more common style?

Answer 1

Programming / detail review

Fan(Fan *peer__ = NULL);

(and some more occurrences)

Identifiers that contain a double underscore are reserved for the compiler/libary ("C++ implementation").

The NULL macro can be thought of as deprecated. Use the nullptr keyword instead. It is more typesafe.

static Fan * create(Fan *peer = NULL);

Since those pointers you return from create own a resource (dynamically allocated memory / an object with dynamic storage duration), you should consider making them smart pointers:

static std::unique_ptr<Fan> create(Fan *peer = nullptr);

The function parameter can remain a raw pointer, since it does not convey/transfer ownership. One could argue that some dumb pointer wrapper that explicitly states it does not own a resource might be more appropriate.

Inproc(std::unique_ptr<Fan> bound__,
 std::unique_ptr<Fan> connected__);

In your original version, you used a std::unique_ptr<Fan>&&. Let's examine the alternatives:

void myfunc(std::unique_ptr<Fan>);
void myfunc(std::unique_ptr<Fan>&&);
void myfunc(std::unique_ptr<Fan>&);
void myfunc(std::unique_ptr<Fan> const&);

Only the first guarantees that the unique_ptr you pass in will be empty afterwards. All others do not, by means of (parameter) types, tell you what happens to your pointer. For more details, see the solution of GotW #105: Smart Pointers, Part 3

If you want to always acquire ownership of the pointer argument, you should define your interface using pass-by-value for unique_ptrs.

Let's see how we can refactor Inproc::create() using these changes:

unique_ptr<Inproc> Inproc::create() {
 if (auto bound = Fan::create() ) {
 if (auto connected = Fan::create(bound.get())) {
 return new(std::nothrow) Inproc(std::move(bound), std::move(connected));
 }
 }
 return nullptr;
}

Note that by returning nullptr in all cases of an error, you lose information about what error occurred.

---

Design review

private:
 Fan(Fan *peer__ = NULL);

Derived classes can only copy a Fan, they cannot create a new one. This seems contradictory to the virtual dtor, which seems to provide "support" for inheritance.

static Fan * create(Fan *peer = NULL);

From the code shown, it is unclear to me why you need to restrict Fans to be created via this factory function (is there a pattern name for this?). It certainly decreases reusability of this class.

For Inproc::create(), the situation could even be worse: You cannot test Inproc independently of Fan, since you can't provide some kind of mocked Fan to construct it. Also, it doesn't seem to require the dynamic storage duration of its unique_ptr subobjects; but again, we don't see the whole program here.

nwp's answer reminded me that you might use pointers to indicate an error. I think that is not a good reason to require dynamic storage duration (-> new + pointers). One solution could be a factory function that returns an optional, or that creates objects in-place at a location passed as a parameter. The former can even be used easily for aggregation; neither can easily be used for inheritance. Another option is to introduce an empty / error state, that the object is set to if construction fails. I don't really like the idea of "empty states" for a class, since those weaken the guarantees / invariants it can provide. But it's a common sight to be able to implement e.g. default constructors. Look at std::thread, std::Xstream etc. - they all have default constructors, even though they leave the object in an almost unusable state.

• Goswin von Brederlow: As said I'm trying this out on Inproc. Fan is legacy code. It's also cut down to just the essential stuff used in the example. There is a ton of other functionality that is irelevant to the constructor so I removed that to keep things simple.

Answer 2

I am still not sure what Fan and Inproc are meant to do. I just fail to see the queuing functionality. Also there are function implementations missing, for example for Fan(Fan *peer__ = NULL);.

• The main point of unique_ptr is that it eliminates ownership problems. When you have a unique_ptr you are the only one who has access to an object and do not need to worry about other code. With regular pointers sometimes you must delete them and sometimes you must not, which is confusing. If someone just calls Inproc::create(); it is already a memory leak which unique_ptrs were meant to prevent:

  std::unique_ptr<Inproc> create();
  

• Avoid functions such as create, init, destroy, copy and clone. C++ has special member functions for that: Constructor, destructor and copy assignment. They cannot always be used, but if they can prefer them. Inpoc::create should actually be a regular constructor. If the construction of an Inproc can fail and you cannot use exceptions provide a member function bool is_valid() const that the caller can use to check if construction worked.

• Prefer nullptr over NULL because nullptr works correctly with templates whereas NULL will occasionally fail.

• Avoid using new and delete. To create a unique_ptr use make_unique and for shared_ptr use make_shared. Somehow the C++11 standard forgot to specify make_unique so if there is no std::make_unique in your STL write your own. You can even write a make_unique_nothrow for your special requirement.

For the questions:

• Would it be possible to have Inproc(Fan &&bound__, Fan &&connected__); as constructor and still use std::unique_ptr and std::move?
  You can express that:

  Inproc(Fan &&bound__, Fan &&connected__);
  std::unique_ptr<Inproc> Inproc::create(){
   std::unique_ptr<Fan> bound(Fan::create());
   if (bound){
   std::unique_ptr<Fan> connected(Fan::create(bound.get()));
   if (connected){
   //return new(std::nothrow) Inproc(std::move(*bound), std::move(*connected));
   return make_unique_nothrow<Inproc>(std::move(*bound), std::move(*connected));
   }
   }
   return nullptr;
  }
  

  (削除) There is no need for std::move here because *bound.release() is already a temporary object. (削除ここまで) I screwed that one up. (削除) Apparently you do need the move even though * unique_ptr.release() is logically a temporary. (削除ここまで) Thanks to @dyp for pointing it out. release is completely misplaced, that would create a memory leak. This time I tested it.

• ... if Inproc() takes std::unique_ptr<Fan>& then the std::move in create becomes uneccessary. Is that a more common style?
  I dislike unique_ptr & as a parameter. Imagine you want to pass an object that is on the stack. The code bloats and becomes inefficient if you have to do that. Instead pass a regular pointer if the resource is optional and a reference if it is not. Also use const if it makes sense. I would also recommend against std::unique_ptr &&, because if is more typing and more mental stress than just std::unique_ptr while the meaning is the same.

• Goswin von Brederlow: "The main point of unique_ptr is that it eliminates ownership problems." That is exactly what I want to do.
• Goswin von Brederlow: "Inpoc::create should actually be a regular constructor." It can't be a normal constructor since I need to be able to signal errors. The existing code is using dependency inversion instead of creating dummy objects with is_good() functions. Changing that would be a major redesign.
• Goswin von Brederlow: As to std::make_unique. My c++ reference has std::unique_ptr(...) as constexpr so it is safe to use without exceptions. No such garantee on std::make_unique. Plus that needs a constructor which often conflicts with the dependency inversion and create factories in this code. Otherwise I will use it though.

Answer 3

If you go for std::unique_ptr -- which is a good thing -- I suggest you to do it consistently. That is, your class Fan should rather look like this:

class Fan {
public:
 static std::unique_ptr<Fan> create(Fan *peer = nullptr);
 virtual ~Fan() = default;
private:
 Fan(Fan *peer = nullptr);
 std::unique_ptr<Fan> _peer;
};

Several things to note here:

1. Use std::unique_ptr<Fan> peer_ as a member of the class. (or a shared_ptr -- which one depends on your requirements). Do not use a raw pointer. Why? If you copy your class via the compiler-created copy constructor, the pointer is simply copied. Now two pointers point to the same object. If one class goes out of scope, it usually calls delete to destruct the object. But then the other class points to nothing, which is a problem. Otherwise, if you don't call delete, none of the two objects frees the memory an thus you get an instance of the famous memory leak.

   This is why smart pointers were created. std::unique_ptr says "that object is mine". Therefore, you can't copy a unique_ptr, and thus the compiler also won't generate a compiler generated copy constructor and assignment operator. This, the above problem is solved trivially, as there never will be two classes which both want to call delete -- there is always a single owner.

   On the other hand, std::shared_ptr allows to share the object among many owners. It avoids the above problem by keeping track how many point to the pointed-to object. Only when nobody needs it anymore, it will call delete once.

   Thus keep in mind to use raw pointers only when you have a good reason. Such one is, when the pointer is only there to observe the pointed-to object. As such it will never call delete nor will do any other kind of memory management.

   Once you settled for one smart pointer, it is often the case that the compiler generated default function are sufficient. Therefore, I set the virtual destructor to the compiler generator default implementation. (Otherwise, if you'd really planned some implementation, you would have to regard the "rule of three").

2. Return a std::unique_ptr from your factory function. Remember, the one who calls create gets a completely new Fan object. He owns it, and is responsible for its memory management. This is exactly what std::unique_ptr does: it possesses the pointed-to-object and takes it with it into his grave (less dramatically: it calls the destructor or Fan once it goes out of scope).

3. Next, you should be aware how you call the constructor and also create. In your case, you pass a raw pointer to both. In each case, the unique_ptr<Fan> _peer inside Fan then will take the ownership of the pointed-to object. So don't use something which is already owned (i.e., already managed by a smart pointer). At best you only use it directly with new Fan(), or better with std::make_unique.

4. Just to note, you should have a good reason why at all you have such a create function. The constructor is often sufficient, and the outside world determines the memory management. Such a create function is often delegated to a factory class.

5. Use nullptr instead of NULL. nullptr in C++11 is especially designed for this purpose (whereas NULL in seldom encountered cases can lead to errors.

6. I would step back from using double underscores __, as they are often used by the compiler. Either use one underscore inside your class, i.e. peer_ or _peer and none in the initialization peer, or make it the other way round.

For the Inproc class, similar things as above hold.

• dyp: I think it's quite likely Fan::peer_ is an observing pointer: Note how Inproc stores two unique_ptrs, where one is supposed to refer to the other one.
• davidhigh: @dyp: I admit I have no idea of the purpose of these two classes. I just reviewed this one class Fan and wrote what came to my mind...
• davidhigh: @dyp: I guess you're right. The OP expicitly says he's trying new things in the class Inproc. Forgot this during my write-diarrhea.

Answer 4

Take 2

I think I've incorporated all the comments. If I missed something please remind me below.

I changed the Fan class to use std::unique_ptr too and added some code to the skeleton. But let me mention again that it's just a sekelton for some Resource that needs to be moved into Inproc. It doesn't do anything as is.

I changed NULL to nullptr and set the copy constructor/assignment to delete since copying the resources is not allowed. I also added some comment and added debug output and a main() function so you can run it and see what goes on. The first example would compile but couldn't be run.

I couldn't get std::make_unique to work since the constructor is private.

I changed *__ to *init to avoid the reserved namespace. I can't use plain peer since that would shadow the peer() function. So member variables are * and initializers are *_init across the board.

#include <memory>
#include <iostream>
#include <cassert>
#define START std::cout << __PRETTY_FUNCTION__ << std::endl
#define END std::cout << __PRETTY_FUNCTION__ << " done" << std::endl
class Fan {
public:
 // Factory with dependency inversion, creates all resource before calling
 // the constructor
 static std::unique_ptr<Fan> create(Fan *peer_init = nullptr) {
 START;
 // some resource allocations left out for simplicity
 // epoll file descriptor
 // read and write ready and blocked queues
 // if they fail create returns nullptr
 // error: 'Fan::Fan(Fan*)' is private
 // std::unique_ptr<Fan> fan = std::make_unique<Fan>(peer_init);
 std::unique_ptr<Fan> fan(new(std::nothrow) Fan(peer_init));
 END;
 return fan;
 }
 ~Fan() {
 START;
 std::cout << " this = " << this << " peer = " << peer_ << std::endl;
 if (peer_) peer(nullptr);
 END;
 }
private:
 // no copying
 Fan(const Fan &) = delete;
 Fan & operator =(const Fan &) = delete;
 // set/reset peer_ pointer
 void peer(Fan *peer_init) {
 START;
 std::cout << " before: this = " << this << " peer = " << peer_ << std::endl;
 if (peer_init) { // finish linking peers
 assert(!peer_); // must not have an observer
 assert(peer_init->peer_ == this); // must already be observed by peer
 peer_ = peer_init;
 } else { // break peers apart
 assert(peer_); // must have an observer
 assert(peer_->peer_ == this); // must be observed by peer
 peer_->peer_ = nullptr;
 peer_ = nullptr;
 }
 std::cout << " after: this = " << this << " peer = " << peer_ << std::endl;
 END;
 }
 // constructor connects to other peer if given
 // other resources left out for simplicity
 Fan(Fan * peer_init /*, resource, resource */) : peer_(peer_init) {
 START;
 if (peer_) {
 assert(!peer_->peer_);
 peer_->peer(this);
 }
 END;
 };
 Fan *peer_; // peer on the other side
};
class Inproc {
public:
 // Factory with dependency inversion, creates all resource before calling
 // the constructor
 static std::unique_ptr<Inproc> create() {
 START;
 // create first Fan of a pair
 if (auto bound = Fan::create()) {
 // create second Fan connected to the first
 if (auto connected = Fan::create(bound.get())) {
 // create Inproc with all resource pre-allocated
 /* error: 'Inproc::Inproc(std::unique_ptr<Fan>,
 std::unique_ptr<Fan>)' is private
 std::unique_ptr<Inproc> inproc =
 std::make_unique<Inproc>(std::move(bound),
 std::move(connected));
 */
 std::unique_ptr<Inproc> inproc(
 new(std::nothrow) Inproc(std::move(bound),
 std::move(connected)));
 // verify that ownership has moved on
 std::cout << " bound is now: " << bound.get() << std::endl;
 END;
 return inproc;
 }
 }
 END;
 return std::unique_ptr<Inproc>(nullptr);
 }
 ~Inproc() {
 START;
 // nothing to do now anymore
 END;
 }
private:
 // constructor takes over ownership of resources
 Inproc(std::unique_ptr<Fan> bound__, std::unique_ptr<Fan> connected__)
 : bound_(std::move(bound__)), connected_(std::move(connected__)) {
 START;
 // other stuff left out for simplicity
 END;
 }
 // no copying
 Inproc(const Inproc &) = delete;
 Inproc & operator =(const Inproc &) = delete;
 // resources
 std::unique_ptr<Fan> bound_;
 std::unique_ptr<Fan> connected_;
};
// test
int main() {
 std::unique_ptr<Inproc> inproc = Inproc::create();
 std::cout << __PRETTY_FUNCTION__ << ": cleaning up" << std::endl;
 inproc.reset();
}

• Davislor: One comment @davidhigh made in passing that I really want to point out: Fan::create(ptr) can be replaced with std::unique_ptr<Fan>(ptr) and std::unique_ptr<Fan> fan(new(std::nothrow) Fan(peer_init)) by auto fan = std::make_unique<Fan>(peer_init). One of these is nearly always the factory function you actually want. You can assign the return value to a std::unique_ptr<BaseClass> or a std::shared_ptr if need be.