remove move constructor for not_null.

[JordanMaples]

Moving not_null will replace whatever pointer is stored within with null, which goes against the purpose of not_null.
This change removes the move constructor for not_null.

[hsutter]

Looks good! Thanks Jordan.

[JordanMaples]

The Travis failure was due to a setup failure. Appveyor, however, was clean so I'm merging this.

[yuri-sevatz]

@hsutter @JordanMaples

I'm not sure if this commit will have the intended effect. Doesn't the constructor need to be marked 'delete' in order for the compiler to stop auto-generating it? (copy constructor is defaulted, so compiler will assume it can safely generate a move-constructor!).

I'm also looking at the move-assignment operator and thinking that it too will still be auto-generated, because the copy-assignment operator is defaulted as well.

That said, I've been watching the problem of move constructors in gsl::not_null and its history within the GSL over the last few months, and this looks like no matter what you pick, it's going to be messy either way.

Either:

A) Disallow move-constructing and move-assigning out of variables, and lose the ability to pass not_null<unique_ptr> and not_null<shared_ptr> by value to functions without having to force people to create l-values on a separate line. Returning not_null<unique_ptr> and not_null<shared_ptr> from a function by value will be almost impossible, with the latter being the only one you'll ever be able to compile, whereas the former with unique_ptr won't!

B) Allow move-constructing and move-assigning out of variables, at the expense of allowing developers to cause a not_null to become null.

Most of this seems to stem from the fact that the language gives no way for implementations to know at compile-time whether we are "moving to pillage an l-value" vs "moving from a temporary r-value reference". I believe this is the crux of why this problem has perplexed everyone working on this class over the last 2 years.

I'd love to know your thinking on this. Personally I'm not sure which of these two is the lesser evil. I'm somewhat inclined to think developers will push for (A) more, but (B) seems like something we'd still want to offer some sort of protection from, at least in contractual places like function calls and function returns via forcing another null check during move-copies and move-assigns.

void foo(not_null<std::shared_ptr<int>> bar) { ... }
not_null<std::shared_ptr<int>> ptr = std::make_shared<int>(5);
...
foo(std::make_shared<int>(int{5})); // Needs (A)
foo(ptr); // OK
foo(std::move(ptr)); // Needs (B)

Maybe if you did this (and implemented A and B), while continuing to support the nullable not_null via move constructors, it might be practically defensive enough to work with that as a known issue?

[beinhaerter]

Unit tests have been changed for not_null and strict_not_null, but only not_null move ctor was removed and strict_not_null(strict_not_null&& other) = default; was left unchanged. Is that on purpose?

[JordanMaples]

@beinhaeter I missed strict_not_null. I'll submit a pull request to align them after I get a chance to discuss @yuri-sevatz's comment with @hsutter and @gdr-at-ms.

[hsutter]

I agree that [strict_]not_null move construction and move assignment should be =delete. Thanks for pointing this out!

Yes, an intended effect of this change is that a not_null<unique_ptr<T>> can only sit there, it can't be moved anywhere. But this is already inherently true, moving one of those is impossible today without breaking the not_null invariant. The correct long-term answer for this would be if C++ gets something along the lines of the relocation / destructive move semantics proposals, where roughly "relocation/destructive-move leaves an object that is guaranteed to be no longer used" or similar (in those proposals, including even its dtor won't be called), then that would naturally enable cases like returning a local not_null<unique_ptr<T>> by value.

[jwakely]

@yuri-sevatz

I'm not sure if this commit will have the intended effect. Doesn't the constructor need to be marked 'delete' in order for the compiler to stop auto-generating it? (copy constructor is defaulted, so compiler will assume it can safely generate a move-constructor!).

This is wrong. A user-declared copy constructor prevents the implicit declaration of the move constructor. A user-declared copy assignment operator also prevents the implicit declaration of a move constructor. See [class.copy.ctor] p8.

I'm also looking at the move-assignment operator and thinking that it too will still be auto-generated, because the copy-assignment operator is defaulted as well.

Wrong, for similar reasons. See [class.copy.assign] p4.

@hsutter

I agree that [strict_]not_null move construction and move assignment should be =delete. Thanks for pointing this out!

No.

NEVER delete a move constructor, it prevents you from copying rvalues.

Consider:
https://godbolt.org/z/145YM9
With GSL 3.0.0 this works perfectly, copying the shared_ptr. With GSL 2.1.0 it fails, because it moves the shared_ptr and leaves the source object empty, violating the contract.

If you delete the move constructor it won't even compile.

Never delete a move constructor or move assignment operator. If you want moving to be equivalent to copying then do not write move ctor/assignment, and ensure there is a user-declared copy ctor or copy assignment operator or both (or a user-declared destructor) N.B. user-declared not user-provided, which means not_null(const not_null&) = default; is fine.

#862 should be closed because it's based on nonsense.

[jwakely]

tl;dr what you want is "no move constructor", not "deleted move constructor".

[jwakely]

@hsutter

Yes, an intended effect of this change is that a not_null<unique_ptr<T>> can only sit there, it can't be moved anywhere. But this is already inherently true, moving one of those is impossible today without breaking the not_null invariant. The correct long-term answer for this would be if C++ gets something along the lines of the relocation / destructive move semantics proposals, where roughly "relocation/destructive-move leaves an object that is guaranteed to be no longer used" or similar (in those proposals, including even its dtor won't be called), then that would naturally enable cases like returning a local not_null<unique_ptr<T>> by value.

Those questionable proposals are not necessary. It works perfectly today with a one line change to gsl/pointers:

--- a/include/gsl/pointers
+++ b/include/gsl/pointers
@@ -72,7 +72,7 @@ public:
     }
 
     template <typename = std::enable_if_t<!std::is_same<std::nullptr_t, T>::value>>
-    constexpr not_null(T u) : ptr_(u)
+    constexpr not_null(T u) : ptr_(std::move(u))
     {
         Expects(ptr_ != nullptr);
     }

Thanks to C++17 guaranteed copy elision you can return a gsl::not_null<std::unique_ptr<X>> from a function and there are no copies. The only thing preventing it working is that the not_null(T) constructor performs a copy from its function parameter, which means you can't use move-only types (and is bad even for movable types, e.g. it causes an unnecessary atomic increment/decrement pair for shared_ptr).

What doesn't work is returning the object from get() (or any of the other members that are implemented in terms of get(), including operator* even though that actually could work fine if it just stopped using get()).

You should look into how move-only iterators are now supported by the C++20 ranges library, so that get() will do different things for copyable or move-only types.

[jwakely]

Aside: why does that constructor have an enable_if constraint to prevent you using that constructor to create a not_null<nullptr_t>? Why not just put that constraint at class scope?

[jwakely]

This would allow not_null<unique_ptr<X>> to be returned from functions and accessed:

--- a/include/gsl/pointers
+++ b/include/gsl/pointers
@@ -72,7 +72,7 @@ public:
     }
 
     template <typename = std::enable_if_t<!std::is_same<std::nullptr_t, T>::value>>
-    constexpr not_null(T u) : ptr_(u)
+    constexpr not_null(T u) : ptr_(std::move(u))
     {
         Expects(ptr_ != nullptr);
     }
@@ -85,14 +85,14 @@ public:
     not_null(const not_null& other) = default;
     not_null& operator=(const not_null& other) = default;
 
-    constexpr T get() const
+    constexpr std::conditional_t<std::is_copy_constructible<T>::value, T, const T&> get() const
     {
         Ensures(ptr_ != nullptr);
         return ptr_;
     }
 
     constexpr operator T() const { return get(); }
-    constexpr T operator->() const { return get(); }
+    constexpr decltype(auto) operator->() const { return get(); }
     constexpr decltype(auto) operator*() const { return *get(); }
 
     // prevents compilation when someone attempts to assign a null pointer constant

I don't think changing operator T() const is needed. It means you can't implicitly convert from not_null<unique_ptr<X>> to const unique_ptr<X>& but that seems fine to me. Preferable even.

[jwakely]

If you don't want get() to return a reference, you can still support not_null<unique_ptr<X>> by putting the Expects check directly in operator-> and operator* and making them not use ptr_ instead of get(). That would still allow you to construct the type and dereference it, which should cover most uses.

[jwakely]

See also #89

[gdr-at-ms]

@jwakely

Never delete a move constructor or move assignment operator. If you want moving to be equivalent to copying then do not write move ctor/assignment, and ensure there is a user-declared copy ctor or copy assignment operator or both (or a user-declared destructor) N.B. user-declared not user-provided, which means not_null(const not_null&) = default; is fine.

Should that be a rule in the Core Guidelines?

[jwakely]

Yes, if people are suggesting doing it, I think it's worth clearly stating why it's wrong.

[hsutter]

@jwakely Thanks for this, your changes look right. Thanks!