-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[libc++] Fix {deque,vector}::append_range assuming too much about the types #162438
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -309,7 +309,7 @@ void sequence_container_benchmarks(std::string container) { | |||||
} | ||||||
|
||||||
///////////////////////// | ||||||
// Variations of push_back | ||||||
// Appending elements | ||||||
///////////////////////// | ||||||
static constexpr bool has_push_back = requires(Container c, ValueType v) { c.push_back(v); }; | ||||||
static constexpr bool has_capacity = requires(Container c) { c.capacity(); }; | ||||||
|
@@ -399,6 +399,53 @@ void sequence_container_benchmarks(std::string container) { | |||||
st.ResumeTiming(); | ||||||
} | ||||||
}); | ||||||
|
||||||
#if TEST_STD_VER >= 23 | ||||||
for (auto gen : generators) | ||||||
bench("append_range()" + tostr(gen), [gen](auto& state) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Or do we want to instead benchmark it with some elements already in the sequence? |
||||||
auto const size = state.range(0); | ||||||
std::vector<ValueType> in; | ||||||
std::generate_n(std::back_inserter(in), size, gen); | ||||||
DoNotOptimizeData(in); | ||||||
|
||||||
Container c; | ||||||
DoNotOptimizeData(c); | ||||||
for (auto _ : state) { | ||||||
c.append_range(in); | ||||||
DoNotOptimizeData(c); | ||||||
|
||||||
state.PauseTiming(); | ||||||
c.clear(); | ||||||
state.ResumeTiming(); | ||||||
} | ||||||
}); | ||||||
#endif | ||||||
} | ||||||
|
||||||
///////////////////////// | ||||||
// Prepending elements | ||||||
///////////////////////// | ||||||
static constexpr bool has_prepend_range = requires(Container c, std::vector<ValueType> v) { c.prepend_range(v); }; | ||||||
|
||||||
if constexpr (has_prepend_range) { | ||||||
for (auto gen : generators) | ||||||
bench("prepend_range()" + tostr(gen), [gen](auto& state) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Same comment as above. |
||||||
auto const size = state.range(0); | ||||||
std::vector<ValueType> in; | ||||||
std::generate_n(std::back_inserter(in), size, gen); | ||||||
DoNotOptimizeData(in); | ||||||
|
||||||
Container c; | ||||||
DoNotOptimizeData(c); | ||||||
for (auto _ : state) { | ||||||
c.prepend_range(in); | ||||||
DoNotOptimizeData(c); | ||||||
|
||||||
state.PauseTiming(); | ||||||
c.clear(); | ||||||
state.ResumeTiming(); | ||||||
} | ||||||
}); | ||||||
} | ||||||
|
||||||
///////////////////////// | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -87,6 +87,11 @@ constexpr void for_all_iterators_and_allocators(Func f) { | |
f.template operator()<Iter, sentinel_wrapper<Iter>, min_allocator<T>>(); | ||
f.template operator()<Iter, sentinel_wrapper<Iter>, safe_allocator<T>>(); | ||
|
||
f.template operator()<Iter, sized_sentinel<Iter>, std::allocator<T>>(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if this can have an adverse effect on our tests compile-time. Yes it increases coverage, but the only row in the cross product that we really want to test is |
||
f.template operator()<Iter, sized_sentinel<Iter>, test_allocator<T>>(); | ||
f.template operator()<Iter, sized_sentinel<Iter>, min_allocator<T>>(); | ||
f.template operator()<Iter, sized_sentinel<Iter>, safe_allocator<T>>(); | ||
|
||
if constexpr (std::sentinel_for<Iter, Iter>) { | ||
f.template operator()<Iter, Iter, std::allocator<T>>(); | ||
f.template operator()<Iter, Iter, test_allocator<T>>(); | ||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -643,6 +643,62 @@ constexpr void test_sequence_assign_range_move_only() { | |||
c.assign_range(in); | ||||
} | ||||
|
||||
struct InPlaceOnly { | ||||
constexpr InPlaceOnly() {} | ||||
InPlaceOnly(const InPlaceOnly&) = delete; | ||||
InPlaceOnly(InPlaceOnly&&) = delete; | ||||
InPlaceOnly& operator=(const InPlaceOnly&) = delete; | ||||
InPlaceOnly& operator=(InPlaceOnly&&) = delete; | ||||
}; | ||||
|
||||
struct EmplaceConstructible { | ||||
EmplaceConstructible(const EmplaceConstructible&) = delete; | ||||
EmplaceConstructible& operator=(const EmplaceConstructible&) = delete; | ||||
EmplaceConstructible& operator=(EmplaceConstructible&&) = delete; | ||||
EmplaceConstructible(EmplaceConstructible&&) = delete; | ||||
constexpr EmplaceConstructible(const InPlaceOnly&) {} | ||||
}; | ||||
|
||||
template <template <class...> class Container> | ||||
philnik777 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
constexpr void test_sequence_append_range_emplace_constructible() { | ||||
InPlaceOnly input[5]; | ||||
types::for_each(types::cpp20_input_iterator_list<InPlaceOnly*>{}, [&]<class Iter> { | ||||
std::ranges::subrange in(Iter(input), sentinel_wrapper<Iter>(Iter(input + 5))); | ||||
Container<EmplaceConstructible> c; | ||||
c.append_range(in); | ||||
}); | ||||
} | ||||
|
||||
template <template <class...> class Container> | ||||
constexpr void test_sequence_prepend_range_emplace_constructible() { | ||||
InPlaceOnly input[5]; | ||||
types::for_each(types::cpp20_input_iterator_list<InPlaceOnly*>{}, [&]<class Iter> { | ||||
std::ranges::subrange in(Iter(input), sentinel_wrapper<Iter>(Iter(input + 5))); | ||||
Container<EmplaceConstructible> c; | ||||
c.prepend_range(in); | ||||
}); | ||||
} | ||||
|
||||
// vector has a special requirement that the type also has to be Cpp17MoveInsertable | ||||
|
||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||
struct EmplaceConstructibleAndMoveInsertable { | ||||
EmplaceConstructibleAndMoveInsertable(const EmplaceConstructibleAndMoveInsertable&) = delete; | ||||
EmplaceConstructibleAndMoveInsertable& operator=(const EmplaceConstructibleAndMoveInsertable&) = delete; | ||||
EmplaceConstructibleAndMoveInsertable& operator=(EmplaceConstructibleAndMoveInsertable&&) = delete; | ||||
constexpr EmplaceConstructibleAndMoveInsertable(EmplaceConstructibleAndMoveInsertable&&) {} | ||||
constexpr EmplaceConstructibleAndMoveInsertable(const InPlaceOnly&) {} | ||||
}; | ||||
|
||||
template <template <class...> class Container> | ||||
constexpr void test_sequence_append_range_emplace_constructible_and_move_insertable() { | ||||
InPlaceOnly input[5]; | ||||
types::for_each(types::cpp20_input_iterator_list<InPlaceOnly*>{}, [&]<class Iter> { | ||||
std::ranges::subrange in(Iter(input), sentinel_wrapper<Iter>(Iter(input + 5))); | ||||
Container<EmplaceConstructibleAndMoveInsertable> c; | ||||
c.append_range(in); | ||||
}); | ||||
} | ||||
|
||||
// Exception safety. | ||||
|
||||
template <template <class...> class Container> | ||||
|
Uh oh!
There was an error while loading. Please reload this page.