Page MenuHomePhabricator
Differential D102809 Diff 352119 libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.swap.pass.cpp

Changeset View

Standalone View

View Options

libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.swap.pass.cpp

  • This file was added.
//===----------------------------------------------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
// UNSUPPORTED: c++03, c++11, c++14, c++17
// UNSUPPORTED: libcpp-no-concepts
// UNSUPPORTED: gcc-10
// template<class I>
// unspecified iter_swap;
#include <iterator>
#include <array>
#include <cassert>
#include "./unqualified_lookup_wrapper.h"
#include "test_iterators.h"
using IterSwapT = decltype(std::ranges::iter_swap);
QuuxplusoneUnsubmitted
Done
ReplyInline Actions

Has this been done in other tests already? If I'd seen it, I'd say why are you adding the &.
I would make IterSwapT an alias for decltype(std::ranges::iter_swap), and then I would check both std::invocable_v<IterSwapT, blah> and a few std::invocable_v<IterSwapT&, blah> and std::invocable_v<IterSwapT&&, blah>. (Remember reference-collapsing means that with your current definition, all three of IterSwapT{,&,&&} are equivalent to IterSwapT&, and that strikes me as less-useful-than-it-could-be.)

Quuxplusone: Has this been done in other tests already? If I'd seen it, I'd say why are you adding the `&`.
static_assert(std::semiregular<std::remove_cvref_t<IterSwapT&>>);
ldionneUnsubmitted
Done
ReplyInline Actions

Based on http://eel.is/c++draft/customization.point.object#2, I would just do std::semiregular<std::remove_cv_t<IterSwapT>>.

ldionne: Based on http://eel.is/c++draft/customization.point.object#2, I would just do `std…
struct HasIterSwap {
int &value;
explicit HasIterSwap(int &value) : value(value) { assert(value == 0); }
QuuxplusoneUnsubmitted
Done
ReplyInline Actions

nit: explicit plz

Quuxplusone: nit: `explicit` plz
friend constexpr void iter_swap(HasIterSwap& a, HasIterSwap& b) {
a.value = 1;
QuuxplusoneUnsubmitted
Done
ReplyInline Actions

https://quuxplusone.github.io/blog/2021/04/03/static-constexpr-whittling-knife/#friendness-to-befriend-an-entity :)

Quuxplusone: https://quuxplusone.github.io/blog/2021/04/03/static-constexpr-whittling-knife/#friendness-to…
b.value = 1;
}
friend constexpr void iter_swap(HasIterSwap& a, int& b) {
a.value = 2;
b = 2;
}
};
static_assert( std::is_invocable_v<IterSwapT, HasIterSwap&, HasIterSwap&>);
QuuxplusoneUnsubmitted
Done
ReplyInline Actions

Here and line 54: I'm worried that your implementation of this function is unnecessary and/or indistinguishable from the default ranges::swap, which means you're not actually testing that it's called. How about replacing these implementations with

friend constexpr void iter_swap(HasIterSwap& a, HasIterSwap& b) {
    called1 = true;
}
friend constexpr void iter_swap(HasIterSwap& a, bool& b) {
    called2 = true;
}

and then writing tests that assert(called1 && !called2) (or whatever you expect to happen).
For constexpr-friendliness, you'll probably need to make your constructor explicit HasIterSwap(bool& c1, bool& c2) : called1(c1), called2(c2) {}. Slack me if this doesn't make sense.

Quuxplusone: Here and line 54: I'm worried that your implementation of this function is unnecessary and/or…
zoecarverAuthorUnsubmitted
Done
ReplyInline Actions

Updated to have something similar to this. Let me know what you think.

For the record: this isn't really meant to test that we use the correct overload, though. I was just testing that it calls "the overload" it didn't really cross my mind that there would be any way for us to even call the bool& overload with an argument of type HasIterSwap. Anyway, I think it is a good idea to not have this just behave like an assignment operator or default swap impl.

zoecarver: Updated to have something similar to this. Let me know what you think. For the record: this…
static_assert( std::is_invocable_v<IterSwapT, HasIterSwap&, int&>);
static_assert(!std::is_invocable_v<IterSwapT, int&, HasIterSwap&>);
static_assert( std::is_invocable_v<IterSwapT&, HasIterSwap&, HasIterSwap&>);
static_assert( std::is_invocable_v<IterSwapT&, HasIterSwap&, int&>);
static_assert(!std::is_invocable_v<IterSwapT&, int&, HasIterSwap&>);
static_assert( std::is_invocable_v<IterSwapT&&, HasIterSwap&, HasIterSwap&>);
static_assert( std::is_invocable_v<IterSwapT&&, HasIterSwap&, int&>);
static_assert(!std::is_invocable_v<IterSwapT&&, int&, HasIterSwap&>);
struct NodiscardIterSwap {
[[nodiscard]] friend int iter_swap(NodiscardIterSwap&, NodiscardIterSwap&) { return 0; }
};
void ensureVoidCast(NodiscardIterSwap& a, NodiscardIterSwap& b) { std::ranges::iter_swap(a, b); }
struct HasRangesSwap {
int &value;
explicit HasRangesSwap(int &value) : value(value) { assert(value == 0); }
friend constexpr void swap(HasRangesSwap& a, HasRangesSwap& b) {
a.value = 1;
b.value = 1;
}
friend constexpr void swap(HasRangesSwap& a, int& b) {
a.value = 2;
b = 2;
}
};
struct HasRangesSwapWrapper {
using value_type = HasRangesSwap;
HasRangesSwap &value;
explicit HasRangesSwapWrapper(HasRangesSwap &value) : value(value) {}
HasRangesSwap& operator*() const { return value; }
};
static_assert( std::is_invocable_v<IterSwapT, HasRangesSwapWrapper&, HasRangesSwapWrapper&>);
// Does not satisfy swappable_with, even though swap(X, Y) is valid.
static_assert(!std::is_invocable_v<IterSwapT, HasRangesSwapWrapper&, int&>);
QuuxplusoneUnsubmitted
Done
ReplyInline Actions

Yes — ranges::iter_swap(++i, ++j) is supposed to increment i only once, not twice.
But this is physically obvious. The Standard needs to mention it because the Standard is pretending that these are expressions-not-function-calls. We don't have to test it; you can remove this TODO.

Quuxplusone: Yes — `ranges::iter_swap(++i, ++j)` is supposed to increment `i` only once, not twice. But this…
zoecarverAuthorUnsubmitted
Done
ReplyInline Actions

Interesting, OK, thanks for explaining.

zoecarver: Interesting, OK, thanks for explaining.
static_assert(!std::is_invocable_v<IterSwapT, int&, HasRangesSwapWrapper&>);
struct B;
struct A {
bool value = false;
A& operator=(const B&) {
value = true;
return *this;
};
};
struct B {
bool value = false;
B& operator=(const A&) {
value = true;
return *this;
};
};
struct MoveOnly2;
struct MoveOnly1 {
bool value = false;
MoveOnly1() = default;
MoveOnly1(MoveOnly1&&) = default;
MoveOnly1& operator=(MoveOnly1&&) = default;
MoveOnly1(const MoveOnly1&) = delete;
MoveOnly1& operator=(const MoveOnly1&) = delete;
MoveOnly1& operator=(MoveOnly2 &&) {
value = true;
return *this;
};
};
struct MoveOnly2 {
bool value = false;
MoveOnly2() = default;
MoveOnly2(MoveOnly2&&) = default;
MoveOnly2& operator=(MoveOnly2&&) = default;
MoveOnly2(const MoveOnly2&) = delete;
MoveOnly2& operator=(const MoveOnly2&) = delete;
MoveOnly2& operator=(MoveOnly1 &&) {
value = true;
return *this;
};
};
int main(int, char**) {
ldionneUnsubmitted
Done
ReplyInline Actions

Minor thing: I like to split test different test cases by enclosing them in a different scope. It makes it easier to see that they are different test cases and that they don't share anything between themselves. So I'd do:

{
    int value1 = 0;
    int value2 = 0;
    HasIterSwap a(value1), b(value2);
    std::ranges::iter_swap(a, b);
    assert(value1 == 1 && value2 == 1);
}
{
    int value1 = 0;
    int value2 = 0;
    HasRangesSwap c(value1), d(value2);
    HasRangesSwapWrapper cWrapper(c), dWrapper(d);
    std::ranges::iter_swap(cWrapper, dWrapper);
    assert(value1 == 1 && value2 == 1);
}

// etc...
ldionne: Minor thing: I like to split test different test cases by enclosing them in a different scope.
int value1 = 0;
int value2 = 0;
HasIterSwap a(value1), b(value2);
std::ranges::iter_swap(a, b);
assert(value1 == 1 && value2 == 1);
QuuxplusoneUnsubmitted
Done
ReplyInline Actions

This is the only place you test std::ranges::iter_swap(lvalue, lvalue). Please expand the tests below to test them also with lvalues of type HasRangesSwapWrapper, A*, etc., at least enough to hit each different overload of iter_swap once with at least one lvalue.

You never test std::ranges::iter_swap(lvalue, rvalue) or std::ranges::iter_swap(rvalue, lvalue), but I don't think those need testing, as long as you hit the (lvalue, lvalue) cases.

Quuxplusone: This is the only place you test `std::ranges::iter_swap(lvalue, lvalue)`. Please expand the…
zoecarverAuthorUnsubmitted
Done
ReplyInline Actions

OK. Added a few more lvalue tests to cover the remaining two overloads.

zoecarver: OK. Added a few more lvalue tests to cover the remaining two overloads.
value1 = 0;
value2 = 0;
HasRangesSwap c(value1), d(value2);
HasRangesSwapWrapper cWrapper(c), dWrapper(d);
std::ranges::iter_swap(cWrapper, dWrapper);
assert(value1 == 1 && value2 == 1);
value1 = 0;
value2 = 0;
std::ranges::iter_swap(HasRangesSwapWrapper(c), HasRangesSwapWrapper(d));
assert(value1 == 1 && value2 == 1);
QuuxplusoneUnsubmitted
Done
ReplyInline Actions

It seems like this is testing the order in which std::swap performs its moves — is that right? If so, I don't think it belongs in libcxx/test/std/ at all. Ditto throughout.

Quuxplusone: It seems like this is testing //the order in which// `std::swap` performs its moves — is that…
zoecarverAuthorUnsubmitted
Done
ReplyInline Actions

Hmm, I see what you're saying with L155 specifically. And we probably don't really need this test (or maybe a better test would be arr[0].moves() + arr[1].moves() == 3.

Ditto throughout.

Where else do you see this?

zoecarver: Hmm, I see what you're saying with L155 specifically. And we probably don't really need this…
QuuxplusoneUnsubmitted
Done
ReplyInline Actions

At least lines 159, 162, 165, 168, 171, 174, 177.

Quuxplusone: At least lines 159, 162, 165, 168, 171, 174, 177.
zoecarverAuthorUnsubmitted
Done
ReplyInline Actions

At least lines 159, 162, 165, 168, 171, 174, 177.

These are just checking that the values of the array buff are swapped. I think this is exactly what we want to be checking.

zoecarver: > At least lines 159, 162, 165, 168, 171, 174, 177. These are just checking that the values of…
A e; B f;
A *ePtr = &e;
B *fPtr = &f;
std::ranges::iter_swap(ePtr, fPtr);
assert(e.value && f.value);
MoveOnly1 g; MoveOnly2 h;
std::ranges::iter_swap(&g, &h);
assert(g.value && h.value);
auto arr = std::array<move_tracker, 2>();
std::ranges::iter_swap(arr.begin(), arr.begin() + 1);
assert(arr[0].moves() == 1 && arr[1].moves() == 2);
int buff[2] = {1, 2};
std::ranges::iter_swap(buff + 0, buff + 1);
assert(buff[0] == 2 && buff[1] == 1);
std::ranges::iter_swap(cpp20_input_iterator(buff), cpp20_input_iterator(buff + 1));
assert(buff[0] == 1 && buff[1] == 2);
std::ranges::iter_swap(cpp17_input_iterator(buff), cpp17_input_iterator(buff + 1));
assert(buff[0] == 2 && buff[1] == 1);
std::ranges::iter_swap(forward_iterator(buff), forward_iterator(buff + 1));
assert(buff[0] == 1 && buff[1] == 2);
std::ranges::iter_swap(bidirectional_iterator(buff), bidirectional_iterator(buff + 1));
assert(buff[0] == 2 && buff[1] == 1);
std::ranges::iter_swap(random_access_iterator(buff), random_access_iterator(buff + 1));
assert(buff[0] == 1 && buff[1] == 2);
std::ranges::iter_swap(contiguous_iterator(buff), contiguous_iterator(buff + 1));
assert(buff[0] == 2 && buff[1] == 1);
return 0;
}