Changeset View
Standalone View
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); | |||||
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&>>); | |||||
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); } | |||||
nit: explicit plz Quuxplusone: nit: `explicit` plz | |||||
friend constexpr void iter_swap(HasIterSwap& a, HasIterSwap& b) { | |||||
a.value = 1; | |||||
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&>); | |||||
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). Quuxplusone: Here and line 54: I'm worried that your implementation of this function is unnecessary and/or… | |||||
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&>); | |||||
Yes — ranges::iter_swap(++i, ++j) is supposed to increment i only once, not twice. Quuxplusone: Yes — `ranges::iter_swap(++i, ++j)` is supposed to increment `i` only once, not twice.
But this… | |||||
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**) { | |||||
int value1 = 0; | |||||
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 value2 = 0; | |||||
HasIterSwap a(value1), b(value2); | |||||
std::ranges::iter_swap(a, b); | |||||
assert(value1 == 1 && value2 == 1); | |||||
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… | |||||
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); | |||||
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… | |||||
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.
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… | |||||
At least lines 159, 162, 165, 168, 171, 174, 177. Quuxplusone: 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; | |||||
} |
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.)