This is an archive of the discontinued LLVM Phabricator instance.

Differential D119445

[libc++] Use std::ranges::swap_ranges in swap CPO
AbandonedPublic

Authored by jloser on Feb 10 2022, 7:57 AM.

Details

Reviewers
ldionne
• Quuxplusone
Mordante
philnik
var-const
Group Reviewers
Restricted Project
Summary

As the comment notes, now that we have implemented std::ranges::swap_ranges,
we can apply it when swapping C-style arrays in the swap CPO when there is
support for ranges.

Diff Detail

Event Timeline

jloser requested review of this revision.Feb 10 2022, 7:57 AM
jloser created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2022, 7:57 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a subscriber: libcxx-commits. · View Herald Transcript
ldionne requested changes to this revision.Feb 10 2022, 8:03 AM
ldionne added inline comments.
libcxx/include/__concepts/swappable.h
82–85

I think this is what you want?

This revision now requires changes to proceed.Feb 10 2022, 8:03 AM
philnik added inline comments.Feb 10 2022, 8:03 AM
libcxx/include/__concepts/swappable.h
82–85
jloser updated this revision to Diff 407549.Feb 10 2022, 8:33 AM

Actually use std::ranges::swap_ranges instead of std::swap_ranges.

libcxx/include/__concepts/swappable.h
82–85

Oops, yes. Fixed - thanks!

• Quuxplusone accepted this revision as: • Quuxplusone.Feb 10 2022, 8:47 AM

LGTM if CI is happy. I skimmed the existing tests for ranges::swap and it looks like arrays are covered decently well; but have you also looked at the tests? Do we need any more coverage for this?

Harbormaster completed remote builds in B148757: Diff 407549.Feb 10 2022, 12:04 PM
jloser added a comment.Feb 10 2022, 3:35 PM
In D119445#3311448, @Quuxplusone wrote:

LGTM if CI is happy. I skimmed the existing tests for ranges::swap and it looks like arrays are covered decently well; but have you also looked at the tests? Do we need any more coverage for this?

Looks like CI isn't happy as this introduces a header cycle as shown by https://buildkite.com/llvm-project/libcxx-ci/builds/8635#9ef9a18c-716c-4962-9f54-27108fa79d51. We'll need to fix that.

As for the tests, I briefly looked at the array tests and they seemed OK - nothing immediately came to mind that I'd like to add (at least as a result of us using ranges::swap_ranges for swappable arrays).

ldionne requested changes to this revision.Feb 14 2022, 12:59 PM

This LGTM, however I would like to see this again after rebasing on top of D118736. D118736 is a must-have, and I think both are going to be in conflict, so we might have to postpone doing this. Or we could do something like:

#if defined(_LIBCPP_HAS_NO_INCOMPLETE_RANGES)
  // use for loop
#else
  // use ranges::swap_ranges
#endif

Once we stop using _LIBCPP_HAS_NO_INCOMPLETE_RANGES, we can refactor this into only the #else branch.

This revision now requires changes to proceed.Feb 14 2022, 12:59 PM
jloser added a comment.Feb 14 2022, 1:04 PM
In D119445#3320632, @ldionne wrote:

This LGTM, however I would like to see this again after rebasing on top of D118736. D118736 is a must-have, and I think both are going to be in conflict, so we might have to postpone doing this. Or we could do something like:

#if defined(_LIBCPP_HAS_NO_INCOMPLETE_RANGES)
  // use for loop
#else
  // use ranges::swap_ranges
#endif

Once we stop using _LIBCPP_HAS_NO_INCOMPLETE_RANGES, we can refactor this into only the #else branch.

The #ifdef dance you proposed seems reasonable, and then can clean it up once we stop using _LIBCPP_HAS_NO_INCOMPLETE_RANGES. I haven't had a chance yet to look into fixing the header cycle issue this exposes when including #include <__algorithm/ranges_swap_ranges.h> from <__concepts/swappable.h>. I'll rebase and fix the cycle sometime this week.

jloser updated this revision to Diff 431121.May 20 2022, 9:21 PM
jloser edited the summary of this revision. (Show Details)

Rebase and add #ifdef.

There isn't a "straightforward" way to fix the header cycle since __concepts/movable.h depends on __concepts/swappable.h

Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2022, 9:21 PM
Harbormaster completed remote builds in B165641: Diff 431121.May 20 2022, 9:25 PM
jloser abandoned this revision.May 21 2022, 8:32 AM

Revision Contents

PathSize
libcxx/
include/
__concepts/
6 lines

Diff 431121

libcxx/include/__concepts/swappable.h

//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
// //
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information. // See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
// //
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
#ifndef _LIBCPP___CONCEPTS_SWAPPABLE_H #ifndef _LIBCPP___CONCEPTS_SWAPPABLE_H
#define _LIBCPP___CONCEPTS_SWAPPABLE_H #define _LIBCPP___CONCEPTS_SWAPPABLE_H
#include <__algorithm/ranges_swap_ranges.h>
#include <__concepts/assignable.h> #include <__concepts/assignable.h>
#include <__concepts/class_or_enum.h> #include <__concepts/class_or_enum.h>
#include <__concepts/common_reference_with.h> #include <__concepts/common_reference_with.h>
#include <__concepts/constructible.h> #include <__concepts/constructible.h>
#include <__config> #include <__config>
#include <__utility/exchange.h> #include <__utility/exchange.h>
#include <__utility/forward.h> #include <__utility/forward.h>
#include <__utility/move.h> #include <__utility/move.h>
▲ Show 20 Lines • Show All 50 Lines • ▼ Show 20 Lines struct __fn {
} }
// 2.2 Otherwise, if `E1` and `E2` are lvalues of array types with equal extent and... // 2.2 Otherwise, if `E1` and `E2` are lvalues of array types with equal extent and...
template<class _Tp, class _Up, size_t _Size> template<class _Tp, class _Up, size_t _Size>
requires __swappable_arrays<_Tp, _Up, _Size> requires __swappable_arrays<_Tp, _Up, _Size>
constexpr void operator()(_Tp(& __t)[_Size], _Up(& __u)[_Size]) const constexpr void operator()(_Tp(& __t)[_Size], _Up(& __u)[_Size]) const
noexcept(noexcept((*this)(*__t, *__u))) noexcept(noexcept((*this)(*__t, *__u)))
{ {
// TODO(cjdb): replace with `ranges::swap_ranges`. #if defined(_LIBCPP_HAS_NO_INCOMPLETE_RANGES)
for (size_t __i = 0; __i < _Size; ++__i) { for (size_t __i = 0; __i < _Size; ++__i) {
(*this)(__t[__i], __u[__i]); (*this)(__t[__i], __u[__i]);
} }
#else
ranges::swap_ranges(__t, __u);
#endif
philnikUnsubmitted
Not Done
ReplyInline Actions
noexcept(noexcept((*this)(*__t, *__u)))
{
- _VSTD::swap_ranges(_VSTD::addressof(__t[0]), _VSTD::addressof(__t[_Size]), _VSTD::addressof(__u[0]));
+ ranges::swap_ranges(std::addressof(__t[0]), std::addressof(__t[_Size]), std::addressof(__u[0]));
}
// 2.3 Otherwise, if `E1` and `E2` are lvalues of the same type `T` that models...
philnik:
ldionneUnsubmitted
Not Done
ReplyInline Actions
noexcept(noexcept((*this)(*__t, *__u)))
{
- _VSTD::swap_ranges(_VSTD::addressof(__t[0]), _VSTD::addressof(__t[_Size]), _VSTD::addressof(__u[0]));
+ ranges::swap_ranges(__t, __u);
}
// 2.3 Otherwise, if `E1` and `E2` are lvalues of the same type `T` that models...

I think this is what you want?

ldionne: I think this is what you want?
jloserAuthorUnsubmitted
Done
ReplyInline Actions

Oops, yes. Fixed - thanks!

jloser: Oops, yes. Fixed - thanks!
} }
// 2.3 Otherwise, if `E1` and `E2` are lvalues of the same type `T` that models... // 2.3 Otherwise, if `E1` and `E2` are lvalues of the same type `T` that models...
template<__exchangeable _Tp> template<__exchangeable _Tp>
constexpr void operator()(_Tp& __x, _Tp& __y) const constexpr void operator()(_Tp& __x, _Tp& __y) const
noexcept(is_nothrow_move_constructible_v<_Tp> && is_nothrow_move_assignable_v<_Tp>) noexcept(is_nothrow_move_constructible_v<_Tp> && is_nothrow_move_assignable_v<_Tp>)
{ {
__y = _VSTD::exchange(__x, _VSTD::move(__y)); __y = _VSTD::exchange(__x, _VSTD::move(__y));
Show All 27 Lines