Skip to content

[libc++] fix flat_set's transparent insert #133402

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Mar 29, 2025
Merged

[libc++] fix flat_set's transparent insert #133402

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3addd27
[libc++] fix `flat_set`'s transparent `insert`
huixie90 Mar 25, 2025
e4540c4
fix
huixie90 Mar 28, 2025
1ad4590
uses std algorithm rather than ranges
huixie90 Mar 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 17 additions & 13 deletions libcxx/include/__flat_set/flat_set.h
View file
Original file line number Diff line number Diff line change
@@ -11,16 +11,16 @@
#define _LIBCPP___FLAT_SET_FLAT_SET_H

#include <__algorithm/lexicographical_compare_three_way.h>
#include <__algorithm/lower_bound.h>
#include <__algorithm/min.h>
#include <__algorithm/ranges_adjacent_find.h>
#include <__algorithm/ranges_equal.h>
#include <__algorithm/ranges_inplace_merge.h>
#include <__algorithm/ranges_lower_bound.h>
#include <__algorithm/ranges_partition_point.h>
#include <__algorithm/ranges_sort.h>
#include <__algorithm/ranges_unique.h>
#include <__algorithm/ranges_upper_bound.h>
#include <__algorithm/remove_if.h>
#include <__algorithm/upper_bound.h>
#include <__assert>
#include <__compare/synth_three_way.h>
#include <__concepts/swappable.h>
@@ -382,7 +382,7 @@ class flat_set {
template <class _Kp>
requires(__is_transparent_v<_Compare> && is_constructible_v<value_type, _Kp>)
_LIBCPP_HIDE_FROM_ABI pair<iterator, bool> insert(_Kp&& __x) {
return emplace(std::forward<_Kp>(__x));
return __emplace(std::forward<_Kp>(__x));
}
_LIBCPP_HIDE_FROM_ABI iterator insert(const_iterator __hint, const value_type& __x) {
return emplace_hint(__hint, __x);
@@ -395,7 +395,7 @@ class flat_set {
template <class _Kp>
requires(__is_transparent_v<_Compare> && is_constructible_v<value_type, _Kp>)
_LIBCPP_HIDE_FROM_ABI iterator insert(const_iterator __hint, _Kp&& __x) {
return emplace_hint(__hint, std::forward<_Kp>(__x));
return __emplace_hint(__hint, std::forward<_Kp>(__x));
}

template <class _InputIterator>
@@ -531,43 +531,47 @@ class flat_set {
}

_LIBCPP_HIDE_FROM_ABI iterator lower_bound(const key_type& __x) {
return iterator(ranges::lower_bound(std::as_const(__keys_), __x, __compare_));
const auto& __keys = __keys_;
return iterator(std::lower_bound(__keys.begin(), __keys.end(), __x, __compare_));
}

_LIBCPP_HIDE_FROM_ABI const_iterator lower_bound(const key_type& __x) const {
return const_iterator(ranges::lower_bound(__keys_, __x, __compare_));
return const_iterator(std::lower_bound(__keys_.begin(), __keys_.end(), __x, __compare_));
}

template <class _Kp>
requires __is_transparent_v<_Compare>
_LIBCPP_HIDE_FROM_ABI iterator lower_bound(const _Kp& __x) {
return iterator(ranges::lower_bound(std::as_const(__keys_), __x, __compare_));
const auto& __keys = __keys_;
return iterator(std::lower_bound(__keys.begin(), __keys.end(), __x, __compare_));
}

template <class _Kp>
requires __is_transparent_v<_Compare>
_LIBCPP_HIDE_FROM_ABI const_iterator lower_bound(const _Kp& __x) const {
return const_iterator(ranges::lower_bound(__keys_, __x, __compare_));
return const_iterator(std::lower_bound(__keys_.begin(), __keys_.end(), __x, __compare_));
}

_LIBCPP_HIDE_FROM_ABI iterator upper_bound(const key_type& __x) {
return iterator(ranges::upper_bound(std::as_const(__keys_), __x, __compare_));
const auto& __keys = __keys_;
return iterator(std::upper_bound(__keys.begin(), __keys.end(), __x, __compare_));
}

_LIBCPP_HIDE_FROM_ABI const_iterator upper_bound(const key_type& __x) const {
return const_iterator(ranges::upper_bound(__keys_, __x, __compare_));
return const_iterator(std::upper_bound(__keys_.begin(), __keys_.end(), __x, __compare_));
}

template <class _Kp>
requires __is_transparent_v<_Compare>
_LIBCPP_HIDE_FROM_ABI iterator upper_bound(const _Kp& __x) {
return iterator(ranges::upper_bound(std::as_const(__keys_), __x, __compare_));
const auto& __keys = __keys_;
return iterator(std::upper_bound(__keys.begin(), __keys.end(), __x, __compare_));
}

template <class _Kp>
requires __is_transparent_v<_Compare>
_LIBCPP_HIDE_FROM_ABI const_iterator upper_bound(const _Kp& __x) const {
return const_iterator(ranges::upper_bound(__keys_, __x, __compare_));
return const_iterator(std::upper_bound(__keys_.begin(), __keys_.end(), __x, __compare_));
}

_LIBCPP_HIDE_FROM_ABI pair<iterator, iterator> equal_range(const key_type& __x) {
@@ -668,7 +672,7 @@ class flat_set {
template <class _Self, class _Kp>
_LIBCPP_HIDE_FROM_ABI static auto __equal_range_impl(_Self&& __self, const _Kp& __key) {
using __iter = _If<is_const_v<__libcpp_remove_reference_t<_Self>>, const_iterator, iterator>;
auto __it = ranges::lower_bound(__self.__keys_, __key, __self.__compare_);
auto __it = std::lower_bound(__self.__keys_.begin(), __self.__keys_.end(), __key, __self.__compare_);
auto __last = __self.__keys_.end();
if (__it == __last || __self.__compare_(__key, *__it)) {
return std::make_pair(__iter(__it), __iter(__it));
176 changes: 70 additions & 106 deletions ...std/containers/container.adaptors/flat.set/flat.set.modifiers/insert_transparent.pass.cpp
View file
Original file line number Diff line number Diff line change
@@ -10,8 +10,8 @@

// <flat_set>

// template<class K> pair<iterator, bool> insert(P&& x);
// template<class K> iterator insert(const_iterator hint, P&& x);
// template<class K> pair<iterator, bool> insert(K&& x);
// template<class K> iterator insert(const_iterator hint, K&& x);

#include <algorithm>
#include <compare>
@@ -27,113 +27,89 @@
#include "test_iterators.h"
#include "min_allocator.h"

// Constraints: is_constructible_v<value_type, K> is true.
// Constraints: The qualified-id Compare::is_transparent is valid and denotes a type. is_constructible_v<value_type, K> is true.
template <class M, class... Args>
concept CanInsert = requires(M m, Args&&... args) { m.insert(std::forward<Args>(args)...); };

using Set = std::flat_set<int>;
using Iter = Set::const_iterator;
using TransparentSet = std::flat_set<int, TransparentComparator>;
using TransparentSetIter = typename TransparentSet::iterator;
static_assert(CanInsert<TransparentSet, ExplicitlyConvertibleTransparent<int>>);
static_assert(CanInsert<TransparentSet, TransparentSetIter, ExplicitlyConvertibleTransparent<int>>);
static_assert(!CanInsert<TransparentSet, NonConvertibleTransparent<int>>);
static_assert(!CanInsert<TransparentSet, TransparentSetIter, NonConvertibleTransparent<int>>);

static_assert(CanInsert<Set, short&&>);
static_assert(CanInsert<Set, Iter, short&&>);
static_assert(!CanInsert<Set, std::string>);
static_assert(!CanInsert<Set, Iter, std::string>);

static int expensive_comparisons = 0;
static int cheap_comparisons = 0;

struct CompareCounter {
int i_ = 0;
CompareCounter(int i) : i_(i) {}
friend auto operator<=>(const CompareCounter& x, const CompareCounter& y) {
expensive_comparisons += 1;
return x.i_ <=> y.i_;
}
bool operator==(const CompareCounter&) const = default;
friend auto operator<=>(const CompareCounter& x, int y) {
cheap_comparisons += 1;
return x.i_ <=> y;
}
};
using NonTransparentSet = std::flat_set<int>;
using NonTransparentSetIter = typename NonTransparentSet::iterator;
static_assert(!CanInsert<NonTransparentSet, ExplicitlyConvertibleTransparent<int>>);
static_assert(!CanInsert<NonTransparentSet, NonTransparentSetIter, ExplicitlyConvertibleTransparent<int>>);
static_assert(!CanInsert<NonTransparentSet, NonConvertibleTransparent<int>>);
static_assert(!CanInsert<NonTransparentSet, NonTransparentSetIter, NonConvertibleTransparent<int>>);

template <class KeyContainer>
void test_one() {
using Key = typename KeyContainer::value_type;
using M = std::flat_set<Key, std::less<Key>, KeyContainer>;
using M = std::flat_set<Key, TransparentComparator, KeyContainer>;

const int expected[] = {1, 2, 3, 4, 5};
{
// insert(P&&)
// Unlike flat_set, here we can't use key_compare to compare value_type versus P,
// so we must eagerly convert to value_type.
M m = {1, 2, 4, 5};
expensive_comparisons = 0;
cheap_comparisons = 0;
std::same_as<std::pair<typename M::iterator, bool>> auto p = m.insert(3); // conversion happens first
assert(expensive_comparisons >= 2);
assert(cheap_comparisons == 0);
assert(p == std::make_pair(m.begin() + 2, true));
assert(std::ranges::equal(m, expected));
}
{
// insert(const_iterator, P&&)
M m = {1, 2, 4, 5};
expensive_comparisons = 0;
cheap_comparisons = 0;
std::same_as<typename M::iterator> auto it = m.insert(m.begin(), 3);
assert(expensive_comparisons >= 2);
assert(cheap_comparisons == 0);
assert(it == m.begin() + 2);
assert(std::ranges::equal(m, expected));
}
{
// insert(value_type&&)
M m = {1, 2, 4, 5};
expensive_comparisons = 0;
cheap_comparisons = 0;
std::same_as<std::pair<typename M::iterator, bool>> auto p = m.insert(3); // conversion happens last
assert(expensive_comparisons >= 2);
assert(cheap_comparisons == 0);
assert(p == std::make_pair(m.begin() + 2, true));
assert(std::ranges::equal(m, expected));
}
{
// insert(const_iterator, value_type&&)
M m = {1, 2, 4, 5};
expensive_comparisons = 0;
cheap_comparisons = 0;
std::same_as<typename M::iterator> auto it = m.insert(m.begin(), 3);
assert(expensive_comparisons >= 2);
assert(cheap_comparisons == 0);
assert(it == m.begin() + 2);
assert(std::ranges::equal(m, expected));
}
{
// emplace(Args&&...)
M m = {1, 2, 4, 5};
expensive_comparisons = 0;
cheap_comparisons = 0;
std::same_as<std::pair<typename M::iterator, bool>> auto p = m.emplace(3); // conversion happens first
assert(expensive_comparisons >= 2);
assert(cheap_comparisons == 0);
assert(p == std::make_pair(m.begin() + 2, true));
assert(std::ranges::equal(m, expected));
const int expected[] = {1, 2, 3, 4, 5};

{
// insert(K&&)
bool transparent_used = false;
M m{{1, 2, 4, 5}, TransparentComparator{transparent_used}};
assert(!transparent_used);
std::same_as<std::pair<typename M::iterator, bool>> decltype(auto) r =
m.insert(ExplicitlyConvertibleTransparent<Key>{3});
assert(transparent_used);
assert(r.first == m.begin() + 2);
assert(r.second);
assert(std::ranges::equal(m, expected));
}
{
// insert(const_iterator, K&&)
bool transparent_used = false;
M m{{1, 2, 4, 5}, TransparentComparator{transparent_used}};
assert(!transparent_used);
std::same_as<typename M::iterator> auto it = m.insert(m.begin(), ExplicitlyConvertibleTransparent<Key>{3});
assert(transparent_used);
assert(it == m.begin() + 2);
assert(std::ranges::equal(m, expected));
}
}

{
// was empty
M m;
std::same_as<std::pair<typename M::iterator, bool>> auto p = m.insert(3);
assert(p == std::make_pair(m.begin(), true));
M expected2 = {3};
assert(std::ranges::equal(m, expected2));
const int expected[] = {3};
{
// insert(K&&)
bool transparent_used = false;
M m{{}, TransparentComparator{transparent_used}};
assert(!transparent_used);
std::same_as<std::pair<typename M::iterator, bool>> decltype(auto) r =
m.insert(ExplicitlyConvertibleTransparent<Key>{3});
assert(!transparent_used); // no elements to compare against
assert(r.first == m.begin());
assert(r.second);
assert(std::ranges::equal(m, expected));
}
{
// insert(const_iterator, K&&)
bool transparent_used = false;
M m{{}, TransparentComparator{transparent_used}};
assert(!transparent_used);
std::same_as<typename M::iterator> auto it = m.insert(m.begin(), ExplicitlyConvertibleTransparent<Key>{3});
assert(!transparent_used); // no elements to compare against
assert(it == m.begin());
assert(std::ranges::equal(m, expected));
}
}
}

void test() {
test_one<std::vector<CompareCounter>>();
test_one<std::deque<CompareCounter>>();
test_one<MinSequenceContainer<CompareCounter>>();
test_one<std::vector<CompareCounter, min_allocator<CompareCounter>>>();
test_one<std::vector<int>>();
test_one<std::deque<int>>();
test_one<MinSequenceContainer<int>>();
test_one<std::vector<int, min_allocator<int>>>();

{
// no ambiguity between insert(pos, P&&) and insert(first, last)
@@ -153,26 +129,14 @@ void test_exception() {
{
auto insert_func = [](auto& m, auto key_arg) {
using FlatSet = std::decay_t<decltype(m)>;
struct T {
typename FlatSet::key_type key_;
T(typename FlatSet::key_type key) : key_(key) {}
operator typename FlatSet::value_type() const { return key_; }
};
T t(key_arg);
m.insert(t);
m.insert(ExplicitlyConvertibleTransparent<typename FlatSet::key_type>{key_arg});
};
test_emplace_exception_guarantee(insert_func);
}
{
auto insert_func_iter = [](auto& m, auto key_arg) {
using FlatSet = std::decay_t<decltype(m)>;
struct T {
typename FlatSet::key_type key_;
T(typename FlatSet::key_type key) : key_(key) {}
operator typename FlatSet::value_type() const { return key_; }
};
T t(key_arg);
m.insert(m.begin(), t);
m.insert(m.begin(), ExplicitlyConvertibleTransparent<typename FlatSet::key_type>{key_arg});
};
test_emplace_exception_guarantee(insert_func_iter);
}
4 changes: 2 additions & 2 deletions libcxx/test/std/containers/container.adaptors/flat.set/helpers.h
View file
Original file line number Diff line number Diff line change
@@ -64,15 +64,15 @@ template <class T, bool ConvertibleToT = false>
struct Transparent {
T t;

operator T() const
explicit operator T() const
requires ConvertibleToT
{
return t;
}
};

template <class T>
using ConvertibleTransparent = Transparent<T, true>;
using ExplicitlyConvertibleTransparent = Transparent<T, true>;

template <class T>
using NonConvertibleTransparent = Transparent<T, false>;