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

<utility>: Side effects in self-swaps of pair are skipped #4597

Closed
frederick-vs-ja opened this issue Apr 17, 2024 · 1 comment · Fixed by #4674
Closed

<utility>: Side effects in self-swaps of pair are skipped #4597

frederick-vs-ja opened this issue Apr 17, 2024 · 1 comment · Fixed by #4674
Labels
bug Something isn't working fixed Something works now, yay!

Comments

@frederick-vs-ja
Copy link
Contributor

frederick-vs-ja commented Apr 17, 2024

Describe the bug

Currently, self-swaps of pair are no-op:

STL/stl/inc/utility

Lines 441 to 460 in 9aca224

_CONSTEXPR20 void swap(pair& _Right) noexcept(
_Is_nothrow_swappable<_Ty1>::value && _Is_nothrow_swappable<_Ty2>::value) {
using _STD swap;
if (this != _STD addressof(_Right)) {
swap(first, _Right.first); // intentional ADL
swap(second, _Right.second); // intentional ADL
}
}
#if _HAS_CXX23
template <int = 0> // see GH-3013
constexpr void swap(const pair& _Right) const
noexcept(is_nothrow_swappable_v<const _Ty1> && is_nothrow_swappable_v<const _Ty2>) {
using _STD swap;
if (this != _STD addressof(_Right)) {
swap(first, _Right.first); // intentional ADL
swap(second, _Right.second); // intentional ADL
}
}
#endif // _HAS_CXX23

However, the Cpp17Swappable requirements ([swappable.requirements]) doesn't seem to exclude self-swaps or side effects in them, and no special case is specified for self-swaps of pair.

Perhaps we should just unconditionally swap members.

Command-line test case

Godbolt link.

#include <utility>

struct swap_counter {
    unsigned int* pcnt_ = nullptr;

    friend constexpr void swap(swap_counter& lhs, swap_counter& rhs) noexcept
    {
        std::swap(lhs.pcnt_, rhs.pcnt_);
        if (lhs.pcnt_ != nullptr)
            ++*lhs.pcnt_;
        if (rhs.pcnt_ != nullptr)
            ++*lhs.pcnt_;
    }
};

static_assert([]
{
    unsigned int cnt{};
    std::pair<swap_counter, int> pr{swap_counter{&cnt}, 0};
    pr.swap(pr);
    return cnt == 2u;
}());

Expected behavior

The code snippet compiles.

@StephanTLavavej StephanTLavavej added the bug Something isn't working label Apr 17, 2024
@StephanTLavavej
Copy link
Member

We looked at this during the weekly maintainer meeting and agree that the Standardese says we should be directly swapping both members without a self-address check.

@CaseyCarter remembered that this is vaguely related to #1485 but is distinct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed Something works now, yay!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants