move ctor for strict_not_null should be removed

In commit 9ff6e19 (issue #743) in 2018 strict_not_null was introduced. The interface was almost identical to the one of not_null, but the constructor is explicit.

Later in commit 6b23937 in 2020 the move ctor for not_null was removed, but the move constructor for strict_not_null was not removed.

In my opinion the move constructor for strict_not_null should be removed as it was done with the move constructor for not_null. This aligns the interfaces of both classes to be the same. And it removes code that looks like it moves but instead copies, which is at least surprising.

maflcko

Thanks to maflcko who brought my attention to it in #1245.

(off-topic: Heh, I still prefer #1238 to make it movable, but this seems possibly controversial and also off-topic here)

on-topic: I agree that removing it is the least confusing and most self-consistent fix for now.

Gabriel Dos Reis (@gdr-at-ms)

Are you wanting the user-defined strict_not_null(strict_not_null&& other) to be = delete;? or to just be removed? The former has already been discussed (and rejected) in #842. The latter is not really a behavior change.

And it removes code that looks like it moves but instead copies, which is at least surprising.

Whether you remove it or keep it, a copy will happen. This behavior is intentional and is already the case with gsl::not_null. https://godbolt.org/z/svh1f8hdd

This aligns the interfaces of both classes to be the same.

I'm happy to accept a PR that makes the interfaces match.

That being said, I interpret the description of #743 as "We introduced gsl::strict_not_null to help customers upgrade". This type is not something specified by the core guidelines (as far as I can tell). Perhaps at this point the best decision is to remove gsl::strict_not_null in the next major GSL release (w/ gsl::dyn_array)?

WDYT? Gabriel Dos Reis (@gdr-at-ms)

Are you wanting ...

Edit: I just saw your PR. "just be removed" is the answer.

That being said, I interpret the description of #743 as "We introduced gsl::strict_not_null to help customers upgrade". This type is not something specified by the core guidelines (as far as I can tell). Perhaps at this point the best decision is to remove gsl::strict_not_null in the next major GSL release (w/ gsl::dyn_array)?

I read 743 as the opposite. It says "using explicit constructor is very beneficial for new code, since it encourages better API design and make null checks intentional." So I think strict_not_null was added to be used in new code, because forcing the explicit behavior onto not_null would break most legacy code using it. So my preference would be to keep it. If it was removed, my recommendation would be to add an option to add or remove explicit from not_null with a macro one way or another, as mentioned in #699 (comment) (GSL_NON_EXPLICIT_NOT_NULL_CTOR)

Fair enough. GitHub code search for "gsl::strict_not_null" has a fair amount of hits, so deleting it is probably not the right call.

I am not sure anymore. The move ctor here checks for is_nothrow_copy_constructible, so maybe there is a good reason to keep it? Yet the different interfaces are strange.

is_nothrow_copy_constructible

I think this is just an artifact of running clang-tidy on the codebase (d69e578)

I'm leaning towards flipping #1247 and adding that interface to gsl::not_null. Basically, I want a defaulted move constructor on gsl::not_null to be noexcept(is_nothrow_copy_constructible).

Motivating example: https://godbolt.org/z/4qbeE8YzK (notice the switch on line 58).

This is certainly a contrived example, but I don't think we want to restrict (as in std::terminate()) users from throwing in their copy constructors.

I'd like to hear other thoughts if there are any. This is a topic I will bring up at the next internal GSL sync (tentatively mid-July).

This is certainly a contrived example, but I don't think we want to restrict (as in std::terminate()) users from throwing in their copy constructors.

I guess this makes sense. While the number of users may be limited, as a library it seems better to be flexible here. However, this is an issue with all other ctors, and I think the move ctor for strict_not_null should still be removed.

For reference, the patch to fix the other ctors noexcept clauses would be:

show patch

diff --git a/include/gsl/pointers b/include/gsl/pointers
index 436f09a..cb9fe13 100644
--- a/include/gsl/pointers
+++ b/include/gsl/pointers
@@ -107,3 +107,3 @@ public:
     template <typename U, typename = std::enable_if_t<std::is_convertible<U, T>::value>>
-    constexpr not_null(U&& u) noexcept(std::is_nothrow_move_constructible<T>::value) : ptr_(std::forward<U>(u))
+    constexpr not_null(U&& u) noexcept(std::is_nothrow_constructible<T, U&&>::value) : ptr_(std::forward<U>(u))
     {
@@ -119,3 +119,3 @@ public:
     template <typename U, typename = std::enable_if_t<std::is_convertible<U, T>::value>>
-    constexpr not_null(const not_null<U>& other) noexcept(std::is_nothrow_move_constructible<T>::value) : not_null(other.get())
+    constexpr not_null(const not_null<U>& other) noexcept(std::is_nothrow_constructible<T, decltype(other.get())>::value) : not_null(other.get())
     {}
@@ -160,3 +160,3 @@ void swap(not_null<T>& a, not_null<T>& b) noexcept
 template <class T>
-auto make_not_null(T&& t) noexcept
+auto make_not_null(T&& t) noexcept(noexcept(not_null<std::remove_cv_t<std::remove_reference_t<T>>>{std::forward<T>(t)}))
 {
@@ -284,3 +284,3 @@ public:
     template <typename U, typename = std::enable_if_t<std::is_convertible<U, T>::value>>
-    constexpr explicit strict_not_null(U&& u) noexcept(std::is_nothrow_move_constructible<T>::value) : not_null<T>(std::forward<U>(u))
+    constexpr explicit strict_not_null(U&& u) noexcept(std::is_nothrow_constructible<T, U&&>::value) : not_null<T>(std::forward<U>(u))
     {}
@@ -288,3 +288,3 @@ public:
     template <typename = std::enable_if_t<!std::is_same<std::nullptr_t, T>::value>>
-    constexpr explicit strict_not_null(T u) noexcept(std::is_nothrow_move_constructible<T>::value) : not_null<T>(std::move(u))
+    constexpr explicit strict_not_null(T u) noexcept(std::is_nothrow_constructible<T, T&&>::value) : not_null<T>(std::move(u))
     {}
@@ -292,3 +292,3 @@ public:
     template <typename U, typename = std::enable_if_t<std::is_convertible<U, T>::value>>
-    constexpr strict_not_null(const not_null<U>& other) noexcept(std::is_nothrow_move_constructible<T>::value) : not_null<T>(other)
+    constexpr strict_not_null(const not_null<U>& other) noexcept(std::is_nothrow_constructible<T, decltype(other.get())>::value) : not_null<T>(other)
     {}
@@ -296,3 +296,3 @@ public:
     template <typename U, typename = std::enable_if_t<std::is_convertible<U, T>::value>>
-    constexpr strict_not_null(const strict_not_null<U>& other) noexcept(std::is_nothrow_move_constructible<T>::value) : not_null<T>(other)
+    constexpr strict_not_null(const strict_not_null<U>& other) noexcept(std::is_nothrow_constructible<T, decltype(other.get())>::value) : not_null<T>(other)
     {}
@@ -335,3 +335,3 @@ strict_not_null<T> operator+(std::ptrdiff_t, const strict_not_null<T>&) = delete
 template <class T>
-auto make_strict_not_null(T&& t) noexcept
+auto make_strict_not_null(T&& t) noexcept(noexcept(strict_not_null<std::remove_cv_t<std::remove_reference_t<T>>>{std::forward<T>(t)}))
 {
diff --git a/tests/pointers_tests.cpp b/tests/pointers_tests.cpp
index fcee2ed..7f791d1 100644
--- a/tests/pointers_tests.cpp
+++ b/tests/pointers_tests.cpp
@@ -95,2 +95,52 @@ TEST(pointers_test, swap)
 
+struct copy_error
+{
+};
+
+struct throwing_copy_ptr
+{
+    int* p;
+
+    explicit throwing_copy_ptr(int* q) noexcept : p(q) {}
+
+    throwing_copy_ptr(const throwing_copy_ptr& other) noexcept(false) : p(other.p)
+    {
+           // Always throw in this test
+        if (other.p != nullptr)
+        {
+            throw copy_error{};
+        }
+    }
+
+    throwing_copy_ptr(throwing_copy_ptr&& other) noexcept : p(other.p)
+    {
+        other.p = nullptr;
+    }
+
+    friend bool operator!=(const throwing_copy_ptr& x, std::nullptr_t) noexcept { return x.p != nullptr; }
+};
+
+template <template <typename> class T>
+bool caught_copy_error() 
+{
+    int i{0};
+    T<throwing_copy_ptr> p{throwing_copy_ptr{&i}};
+    try
+    {
+        auto q = p;
+        (void)q;
+        return false;
+    }
+    catch (const copy_error&)
+    {
+        return true;
+    }
+}
+
+TEST(pointers_test, copy_throws)
+{
+    EXPECT_TRUE(caught_copy_error<gsl::strict_not_null>());
+    EXPECT_TRUE(caught_copy_error<gsl::not_null>());
+}
+
 TEST(pointers_test, member_types)

Navigation Menu

Repository navigation

move ctor for strict_not_null should be removed #1246

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

move ctor for strict_not_null should be removed #1246

Description

Activity

beinhaerter commented on May 26, 2026

maflcko commented on May 26, 2026

carsonRadtke commented on May 27, 2026

carsonRadtke commented on May 27, 2026

maflcko commented on May 28, 2026

carsonRadtke commented on May 28, 2026

beinhaerter commented on May 29, 2026

maflcko commented on May 29, 2026

carsonRadtke commented on May 29, 2026

maflcko commented on Jun 2, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions