Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
christianbrugger opened this issue Aug 6, 2023 · 4 comments · May be fixed by #1160
Open

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

christianbrugger opened this issue Aug 6, 2023 · 4 comments · May be fixed by #1160
Assignees
@carsonRadtke
Labels
feature request

Comments

@christianbrugger
Copy link

christianbrugger commented Aug 6, 2023 •
edited
Loading

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?
@christianbrugger christianbrugger changed the title Provide swap for gsl::not_null for move only types Missing Swap for gsl::not_null for move-only types Aug 6, 2023
@christianbrugger christianbrugger changed the title Missing Swap for gsl::not_null for move-only types Missing swap for gsl::not_null for move-only types Aug 6, 2023
@dmitrykobets-msft
Copy link
Member

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

@hsutter
Copy link
Contributor

hsutter commented Oct 18, 2023 •
edited
Loading

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
Copy link

mbeutel commented Oct 28, 2023 •
edited
Loading

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
Copy link
Contributor

hsutter commented Oct 29, 2023 •
edited
Loading

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

carsonRadtke added a commit to carsonRadtke/GSL that referenced this issue Oct 17, 2024
@carsonRadtke
introduce gsl::swap for swapping gsl::not_null
d988e0e 
fixes: microsoft#1129

* create gsl::swap<T>(T&, T&) which wraps std::swap
* specialize gsl::swap<T>(gsl::not_null<T>&, gsl::not_null<T>&)
* add tests
@carsonRadtke carsonRadtke linked a pull request Oct 17, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@carsonRadtke carsonRadtke

Labels
feature request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

introduce gsl::swap for swapping gsl::not_null carsonRadtke/GSL
6 participants
@christianbrugger @hsutter @mbeutel @carsonRadtke @d-winsor @dmitrykobets-msft