Cppcoreguidelines: Using `not_null` with *smart pointers*.

[galik]

I think this _issue_ really needs to be brought up here first: https://github.com/Microsoft/GSL/issues/89

I made a comment on that _issue_ over at Microsoft/GSL but it occurred to me that it should be dealt with in the Guidelines, so I will repeat my comments here:

According to the guidelines for passing _smart pointers_ it seems that the only reason we should be passing a _smart pointer_ to a function is when that fuction modifies the _ownership_ in some way. Otherwise, the recommendation is that we pass a _raw pointer_.

R.30: Take smart pointers as parameters only to explicitly express lifetime semantics

Reason: Accepting a smart pointer to a widget is wrong if the function just needs the widget itself.
It should be able to accept any widget object, not just ones whose lifetimes are managed by a particular kind of smart pointer.
A function that does not manipulate lifetime should take raw pointers or references instead.

Bearing that in mind I am not sure that not_null should be passing smart pointers into functions at all.

After all a function receiving a not_null generally wants to be able to make calls against the pointer without having to first check its validity. It is unlikely a function receiving a not_null would want to deal with its _ownership_ and, even if it did, would we want to encourage that? After all such a function would have the power to _reset_ the pointer breaking its _not null_ contract.

Maybe there are use-cases I have missed but it seems to me the main use for the not_null (with respect to _smart pointers_) is for _down-calling_ and therefore for passing only the _raw pointer_ that the _smart pointer_ is managing.

Therefore we might not want a not_null to _contain_ a _smart pointer_, only its associated _raw pointer_.

To that effect we could create not_null _specializations_ for both unique_ptr and shared_ptr whereby it captures their managed _raw pointer_ using get() and implements its _not null_ invariant on construction.

[bashrc-real]

A use case where we may want to use the class with smart pointer is passing a shared resource while creating an object.
For eg BufferReader::BufferReader(const shared_ptr &stream) : m_stream(stream) {}
BufferReader may want to (and very rightly so) ensure that it's always created with a valid non-null stream.
Although I agree that get() for smart pointers should not return the actual object, because then handling smart pointers which assume sole ownership of the object becomes difficult. On the other hand creating a specialization for shared_ptr and unique_ptr seems odd since it makes the contract complicated. Probably we need a new class altogether for handling ownership as well as asserting validity of a pointer

[rodiers]

@galik "Therefore we might not want a not_null to contain a smart pointer, only its associated raw pointer"

I see two use-cases:

• Suppose we have some kind of factory class that return creates objects on the heap and transfers ownership to the clients of this factory and hence it return smart pointers (probably unique_ptr). Some factories do want to guarantee that the objects always exist and hence would like to mark it as not_null. It is its post-condition that the object always exists. Clients then don't need to test whether the object is not null.
• A second use-case is a kind of sink class that takes ownership of the object it takes. Hence it only accepts smart pointers. It might not make sense for the sink to accept a nullptr and hence its precondition is that the smart pointer shouldn't be a nullptr. Hence it uses not_null.

[neilmacintosh]

rodiers has given two nice examples of where a function might want to accept or return a smart pointer (it is a factory, or a sink). In those cases, using not_null around a smart pointer would be entirely appropriate, it does not interfere with the fact that a smart pointer is still being passed/returned to express lifetime semantics.

So I think both guidelines are fine as-is: the guideline to only use smart pointers as parameters to express lifetime semantics and the recommendation to use not_null to indicate a particular property of a pointer - raw or smart.

The issue over at the GSL repo seems to be more about the specific characteristics of that implementation of not_null...which is a discussion best had over there.