Using std::unique_ptr and std::move

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 bound__,
           std::unique_ptr connected__);
    std::unique_ptr bound_;
    std::unique_ptr connected_;
};

Inproc * Inproc::create() {
    std::unique_ptr bound(Fan::create());
    if (bound) {
        std::unique_ptr 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 bound__,
               std::unique_ptr connected__)
    : bound_(std::move(bound__)), connected_(std::move(connected__)) { }

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 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 bound__,
       std::unique_ptr connected__);

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

void myfunc(std::unique_ptr);
void myfunc(std::unique_ptr&&);
void myfunc(std::unique_ptr&);
void myfunc(std::unique_ptr 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::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.

StackExchange Code Review Q#70598, answer score: 11