microsoft / GSL

Guidelines Support Library
Other
6.11k stars 736 forks source link

Missing `swap` for `gsl::not_null` for move-only types #1129

Open christianbrugger opened 1 year ago

christianbrugger commented 1 year ago

I want to exchange the content of two gsl::not_null<std::unique_ptr<int>>.

I was pretty sure a swap would work, but couldn't get it working.

#include <memory>
#include <algorithm>

#include <gsl/gsl>

auto main() -> int {

    auto a = gsl::not_null<std::unique_ptr<int>> {std::make_unique<int>(0)};
    auto b = gsl::not_null<std::unique_ptr<int>> {std::make_unique<int>(1)};

    using std::swap;
    swap(a, b);

    return *a;
}

https://godbolt.org/z/64na6e356

This gives an error:

no matching function for call to 'swap(gsl::not_null<std::unique_ptr<int> >&, gsl::not_null<std::unique_ptr<int> >&)'

I understand that its not possible to move from such a pointer. However, not even enabling swap for smart-pointers makes std::not_null even less useful.

Is there anything against implementing a custom swap, that takes two not_null pointers and swaps the underlying, as we know they are both not null?

dmitrykobets-msft commented 1 year ago

Hi @christianbrugger, thanks for the question; I'll bring it up in the next maintainers' sync.

hsutter commented 10 months ago

Maintainers call: Yes, there should be a homogeneous swap. We should add a member not_null<T>::swap(not_null<T>&) and specialize template<typename T> std::swap(not_null<T>&, not_null<T>&).

mbeutel commented 10 months ago

and specialize template<typename T> std::swap(not_null<T>&, not_null<T>&).

Wait, is specializing function templates from namespace std something we do? I thought that was outlawed as a consequence of P0551. The current draft only mentions that class templates from std may be specialized.

Edit: the cppreference.com documentation on std::swap discusses this:

Specializations

std::swap may be specialized in namespace std for program- defined types, but such specializations are not found by ADL (the namespace std is not the associated namespace for the program-defined type). (until C++20)

The expected way to make a program-defined type swappable is to provide a non-member function swap in the same namespace as the type: see Swappable for details.

hsutter commented 10 months ago

Thanks! I think you're correct, we should overload (not specialize) in gsl:: (not std::).