Should not_null<> be two different classes?

[mbeutel]

While reviewing #297 it occurred to me that there are two possible interpretations of the not_null<> type mentioned by the C++ Core Guidelines:

---

not_null_1<>, a building-block type, to be used e.g. as data member or as container element:

class MyResource {
private:
    // std::unique_ptr<RawResource> res_;
    not_null_1<std::unique_ptr<RawResource>> res_;
    ...
public:
};

// std::unordered_map<std::string, std::shared_ptr<RawResource>> resourceRegistry;
std::unordered_map<std::string, not_null_1<std::shared_ptr<RawResource>>> resourceRegistry;

// void consume(std::unique_ptr<Resource> res);
void consume(not_null_1<std::unique_ptr<Resource>> res);

not_null_1<> is copyable if the pointer type is copyable. It is also movable; the move leaves behind a "valid but unspecified" pointer object:

gsl::not_null_1<std::unique_ptr<RawResource>> a = ...;
auto b = std::move(a);
// a is now nullptr

Because C++ doesn't have destructive move, we have to worried about the moved-from state. Although its occurrence cannot be statically prevented¹, not_null_1<> makes sure that the moved-from state doesn't proliferate (⇒ it checks for nullptr in all constructors), and that pointers in moved-from state are not accessed (⇒ it checks for nullptr in accessor functions such as get(), as_nullable(), operator ->()).

---

not_null_2<>, a type with reference/borrow semantics used in function signatures to express a function precondition (that the argument shall not be nullptr) through the type system:

// int length(Record* p);
int length(not_null_2<Record*> p);  // example from C++ Core Guidelines

// std::size_t compute_hash(std::unique_ptr<Resource> const& res);
std::size_t compute_hash(not_null_2<std::unique_ptr<Resource>> res);

not_null_2<P> is effectively P const&; it is copyable, but that just copies the reference to the pointer. (Copy assignment could also be supported by actually storing P const*, not P const&.)

not_null_2<> hence does not need to be movable. It establishes its invariant (pointer is not nullptr) in the explicit not_null_2<P>::not_null_2(P const&) constructor. Not being movable, it has no moved-from state, and hence no further checks are necessary in copy constructors/assignment operators or accessor methods.

Using not_null_2<P> instead of not_null_1<P> const& in function signatures is advantageous because it means the onus of checking the precondition is on the caller:

std::size_t compute_hash_1(not_null_1<std::unique_ptr<Resource>> const& res)
{
    return std::hash<std::unique_ptr<Resource>>{ }(res);  // postcondition failure if `res` is `nullptr`
}

std::size_t compute_hash_2(not_null_2<std::unique_ptr<Resource>> res)  // precondition failure in `not_null_2<>` constructor *at the call site* when constructed from `nullptr`
{
    return std::hash<std::unique_ptr<Resource>>{ }(res);  // cannot fail
}

In this case, compute_hash_2() could even be noexcept even if gsl_CONFIG_CONTRACT_VIOLATION_THROWS is defined because the precondition check is run by the caller before the function is invoked.

---

The examples in the Core Guidelines are too shallow to shed light on which of these semantics was actually intended. gsl-lite currently implements gsl::not_null<> with not_null_1<> semantics. However, I think that not_null_2<> actually resembles more closely what the Guidelines authors had in mind; they liken not_null<> to span<> and seem to imply it has borrow semantics, and all their examples would work fine with not_null_2<>.

I'm pondering whether we want something like not_null_2<> in the library, perhaps named gsl::not_null_arg<>, gsl::not_null_ref<>, or gsl::not_null_view<>. Comments welcome.

¹: Some static analyzers can diagnose obvious cases of use-after-move, e.g. Clang-Tidy.

[mbeutel]

Because C++ doesn't have destructive move, we have to worried about the moved-from state. Although its occurrence cannot be statically prevented¹, not_null_1<> makes sure that the moved-from state doesn't proliferate (⇒ it checks for nullptr in all constructors), and that pointers in moved-from state are not accessed (⇒ it checks for nullptr in accessor functions such as get(), as_nullable(), operator ->()).

I realized that, for raw pointers, moving just copies, hence there is no dedicated moved-from state to worry about. In fact, the postcondition checks in get(), as_nullable(), operator ->() are unnecessary for not_null_1<T*>.

That further reduces the necessity for a type with not_null_2<> semantics; there would be no use for not_null_2<T*> because not_null_1<T*> would do exactly the same. That leaves not_null_2<SmartPtr<T>>, which could act as a semantic substitute for both not_null_1<SmartPtr<T>> const& and SmartPtr<T> const& in function arguments. The Core Guidelines give the following advice about passing smart pointers:

R.30: Take smart pointers as parameters only to explicitly express lifetime semantics
F.7: For general use, take T* or T& arguments rather than smart pointers
R.36: Take a const shared_ptr<widget>& parameter to express that it might retain a reference count to the object ???

The shared_ptr<T> const& use case is arguably a rare corner case, and std::unique_ptr<T> const& isn't mentioned and thus implicitly discouraged.

So perhaps there isn't much use for not_null_2<> at all.

[mbeutel]

I realized that, for raw pointers, moving just copies, hence there is no dedicated moved-from state to worry about. In fact, the postcondition checks in get(), as_nullable(), operator ->() are unnecessary for not_null_1<T*>.

4e0b7d2 addresses this point and adds a specialization of not_null<> for raw pointer types which elides the unnecessary checks.

[mbeutel]

Furthermore, if we ever wanted a not_null<> type with reference/borrow semantics, we could simply make gsl::not_null<> work for reference type arguments:

// std::size_t compute_hash(std::unique_ptr<Resource> const& res);
std::size_t compute_hash(gsl::not_null<std::unique_ptr<Resource> const&> res);

    // "will" or "might" reseat pointer
// void reseat(unique_ptr<widget>&);
void reseat(gsl::not_null<unique_ptr<widget>&>);

    // "might" retain refcount
// void may_share(shared_ptr<widget> const&);
void may_share(gsl::not_null<shared_ptr<widget> const&>);

But for now, let's not. Either way, there is really no need for a dedicated type with not_null_2<> semantics.