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.

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

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

Closed
christianbrugger opened this issue Aug 6, 2023 · 4 comments · Fixed by #1160
Closed

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

christianbrugger opened this issue Aug 6, 2023 · 4 comments · Fixed by #1160
Assignees
Labels
Effort: Low Requires minimal effort Priority: Low Can be handled at leisure Status: In Progress Currently being worked on Type: Enhancement Suggests an improvement or new feature

Comments

@christianbrugger
Copy link

christianbrugger commented Aug 6, 2023

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

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

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

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
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 added Priority: Low Can be handled at leisure Type: Enhancement Suggests an improvement or new feature Status: In Progress Currently being worked on Effort: Low Requires minimal effort and removed feature request labels Dec 2, 2024
@carsonRadtke carsonRadtke moved this from New to Assigned in Microsoft/GSL Active Issues Dec 3, 2024
@carsonRadtke carsonRadtke moved this from Assigned to PR in Microsoft/GSL Active Issues Dec 3, 2024
carsonRadtke added a commit to carsonRadtke/GSL that referenced this issue Dec 13, 2024
@github-project-automation github-project-automation bot moved this from PR to Won't Fix in Microsoft/GSL Active Issues Dec 13, 2024
@carsonRadtke carsonRadtke moved this from Won't Fix to Fixed in Microsoft/GSL Active Issues Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Effort: Low Requires minimal effort Priority: Low Can be handled at leisure Status: In Progress Currently being worked on Type: Enhancement Suggests an improvement or new feature
Projects
Status: Fixed
Development

Successfully merging a pull request may close this issue.

6 participants