Skip to content

Commit e16541e

Browse files
authored
[libc++] fix flat_set's transparent insert (llvm#133402)
Unlike `flat_map` and `flat_multimap`, The two function template overloads `flat_set::insert`'s wording strongly suggest we should use the transparent comparator https://eel.is/c++draft/flat.set#modifiers-1 Both the code and the tests were not using the transparent comparator, which needs to be fixed
1 parent 847cdd4 commit e16541e

File tree

3 files changed

+89
-121
lines changed

3 files changed

+89
-121
lines changed

libcxx/include/__flat_set/flat_set.h

Lines changed: 17 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -11,16 +11,16 @@
1111
#define _LIBCPP___FLAT_SET_FLAT_SET_H
1212

1313
#include <__algorithm/lexicographical_compare_three_way.h>
14+
#include <__algorithm/lower_bound.h>
1415
#include <__algorithm/min.h>
1516
#include <__algorithm/ranges_adjacent_find.h>
1617
#include <__algorithm/ranges_equal.h>
1718
#include <__algorithm/ranges_inplace_merge.h>
18-
#include <__algorithm/ranges_lower_bound.h>
1919
#include <__algorithm/ranges_partition_point.h>
2020
#include <__algorithm/ranges_sort.h>
2121
#include <__algorithm/ranges_unique.h>
22-
#include <__algorithm/ranges_upper_bound.h>
2322
#include <__algorithm/remove_if.h>
23+
#include <__algorithm/upper_bound.h>
2424
#include <__assert>
2525
#include <__compare/synth_three_way.h>
2626
#include <__concepts/swappable.h>
@@ -382,7 +382,7 @@ class flat_set {
382382
template <class _Kp>
383383
requires(__is_transparent_v<_Compare> && is_constructible_v<value_type, _Kp>)
384384
_LIBCPP_HIDE_FROM_ABI pair<iterator, bool> insert(_Kp&& __x) {
385-
return emplace(std::forward<_Kp>(__x));
385+
return __emplace(std::forward<_Kp>(__x));
386386
}
387387
_LIBCPP_HIDE_FROM_ABI iterator insert(const_iterator __hint, const value_type& __x) {
388388
return emplace_hint(__hint, __x);
@@ -395,7 +395,7 @@ class flat_set {
395395
template <class _Kp>
396396
requires(__is_transparent_v<_Compare> && is_constructible_v<value_type, _Kp>)
397397
_LIBCPP_HIDE_FROM_ABI iterator insert(const_iterator __hint, _Kp&& __x) {
398-
return emplace_hint(__hint, std::forward<_Kp>(__x));
398+
return __emplace_hint(__hint, std::forward<_Kp>(__x));
399399
}
400400

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

533533
_LIBCPP_HIDE_FROM_ABI iterator lower_bound(const key_type& __x) {
534-
return iterator(ranges::lower_bound(std::as_const(__keys_), __x, __compare_));
534+
const auto& __keys = __keys_;
535+
return iterator(std::lower_bound(__keys.begin(), __keys.end(), __x, __compare_));
535536
}
536537

537538
_LIBCPP_HIDE_FROM_ABI const_iterator lower_bound(const key_type& __x) const {
538-
return const_iterator(ranges::lower_bound(__keys_, __x, __compare_));
539+
return const_iterator(std::lower_bound(__keys_.begin(), __keys_.end(), __x, __compare_));
539540
}
540541

541542
template <class _Kp>
542543
requires __is_transparent_v<_Compare>
543544
_LIBCPP_HIDE_FROM_ABI iterator lower_bound(const _Kp& __x) {
544-
return iterator(ranges::lower_bound(std::as_const(__keys_), __x, __compare_));
545+
const auto& __keys = __keys_;
546+
return iterator(std::lower_bound(__keys.begin(), __keys.end(), __x, __compare_));
545547
}
546548

547549
template <class _Kp>
548550
requires __is_transparent_v<_Compare>
549551
_LIBCPP_HIDE_FROM_ABI const_iterator lower_bound(const _Kp& __x) const {
550-
return const_iterator(ranges::lower_bound(__keys_, __x, __compare_));
552+
return const_iterator(std::lower_bound(__keys_.begin(), __keys_.end(), __x, __compare_));
551553
}
552554

553555
_LIBCPP_HIDE_FROM_ABI iterator upper_bound(const key_type& __x) {
554-
return iterator(ranges::upper_bound(std::as_const(__keys_), __x, __compare_));
556+
const auto& __keys = __keys_;
557+
return iterator(std::upper_bound(__keys.begin(), __keys.end(), __x, __compare_));
555558
}
556559

557560
_LIBCPP_HIDE_FROM_ABI const_iterator upper_bound(const key_type& __x) const {
558-
return const_iterator(ranges::upper_bound(__keys_, __x, __compare_));
561+
return const_iterator(std::upper_bound(__keys_.begin(), __keys_.end(), __x, __compare_));
559562
}
560563

561564
template <class _Kp>
562565
requires __is_transparent_v<_Compare>
563566
_LIBCPP_HIDE_FROM_ABI iterator upper_bound(const _Kp& __x) {
564-
return iterator(ranges::upper_bound(std::as_const(__keys_), __x, __compare_));
567+
const auto& __keys = __keys_;
568+
return iterator(std::upper_bound(__keys.begin(), __keys.end(), __x, __compare_));
565569
}
566570

567571
template <class _Kp>
568572
requires __is_transparent_v<_Compare>
569573
_LIBCPP_HIDE_FROM_ABI const_iterator upper_bound(const _Kp& __x) const {
570-
return const_iterator(ranges::upper_bound(__keys_, __x, __compare_));
574+
return const_iterator(std::upper_bound(__keys_.begin(), __keys_.end(), __x, __compare_));
571575
}
572576

573577
_LIBCPP_HIDE_FROM_ABI pair<iterator, iterator> equal_range(const key_type& __x) {
@@ -668,7 +672,7 @@ class flat_set {
668672
template <class _Self, class _Kp>
669673
_LIBCPP_HIDE_FROM_ABI static auto __equal_range_impl(_Self&& __self, const _Kp& __key) {
670674
using __iter = _If<is_const_v<__libcpp_remove_reference_t<_Self>>, const_iterator, iterator>;
671-
auto __it = ranges::lower_bound(__self.__keys_, __key, __self.__compare_);
675+
auto __it = std::lower_bound(__self.__keys_.begin(), __self.__keys_.end(), __key, __self.__compare_);
672676
auto __last = __self.__keys_.end();
673677
if (__it == __last || __self.__compare_(__key, *__it)) {
674678
return std::make_pair(__iter(__it), __iter(__it));

libcxx/test/std/containers/container.adaptors/flat.set/flat.set.modifiers/insert_transparent.pass.cpp

Lines changed: 70 additions & 106 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,8 @@
1010

1111
// <flat_set>
1212

13-
// template<class K> pair<iterator, bool> insert(P&& x);
14-
// template<class K> iterator insert(const_iterator hint, P&& x);
13+
// template<class K> pair<iterator, bool> insert(K&& x);
14+
// template<class K> iterator insert(const_iterator hint, K&& x);
1515

1616
#include <algorithm>
1717
#include <compare>
@@ -27,113 +27,89 @@
2727
#include "test_iterators.h"
2828
#include "min_allocator.h"
2929

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

34-
using Set = std::flat_set<int>;
35-
using Iter = Set::const_iterator;
34+
using TransparentSet = std::flat_set<int, TransparentComparator>;
35+
using TransparentSetIter = typename TransparentSet::iterator;
36+
static_assert(CanInsert<TransparentSet, ExplicitlyConvertibleTransparent<int>>);
37+
static_assert(CanInsert<TransparentSet, TransparentSetIter, ExplicitlyConvertibleTransparent<int>>);
38+
static_assert(!CanInsert<TransparentSet, NonConvertibleTransparent<int>>);
39+
static_assert(!CanInsert<TransparentSet, TransparentSetIter, NonConvertibleTransparent<int>>);
3640

37-
static_assert(CanInsert<Set, short&&>);
38-
static_assert(CanInsert<Set, Iter, short&&>);
39-
static_assert(!CanInsert<Set, std::string>);
40-
static_assert(!CanInsert<Set, Iter, std::string>);
41-
42-
static int expensive_comparisons = 0;
43-
static int cheap_comparisons = 0;
44-
45-
struct CompareCounter {
46-
int i_ = 0;
47-
CompareCounter(int i) : i_(i) {}
48-
friend auto operator<=>(const CompareCounter& x, const CompareCounter& y) {
49-
expensive_comparisons += 1;
50-
return x.i_ <=> y.i_;
51-
}
52-
bool operator==(const CompareCounter&) const = default;
53-
friend auto operator<=>(const CompareCounter& x, int y) {
54-
cheap_comparisons += 1;
55-
return x.i_ <=> y;
56-
}
57-
};
41+
using NonTransparentSet = std::flat_set<int>;
42+
using NonTransparentSetIter = typename NonTransparentSet::iterator;
43+
static_assert(!CanInsert<NonTransparentSet, ExplicitlyConvertibleTransparent<int>>);
44+
static_assert(!CanInsert<NonTransparentSet, NonTransparentSetIter, ExplicitlyConvertibleTransparent<int>>);
45+
static_assert(!CanInsert<NonTransparentSet, NonConvertibleTransparent<int>>);
46+
static_assert(!CanInsert<NonTransparentSet, NonTransparentSetIter, NonConvertibleTransparent<int>>);
5847

5948
template <class KeyContainer>
6049
void test_one() {
6150
using Key = typename KeyContainer::value_type;
62-
using M = std::flat_set<Key, std::less<Key>, KeyContainer>;
51+
using M = std::flat_set<Key, TransparentComparator, KeyContainer>;
6352

64-
const int expected[] = {1, 2, 3, 4, 5};
6553
{
66-
// insert(P&&)
67-
// Unlike flat_set, here we can't use key_compare to compare value_type versus P,
68-
// so we must eagerly convert to value_type.
69-
M m = {1, 2, 4, 5};
70-
expensive_comparisons = 0;
71-
cheap_comparisons = 0;
72-
std::same_as<std::pair<typename M::iterator, bool>> auto p = m.insert(3); // conversion happens first
73-
assert(expensive_comparisons >= 2);
74-
assert(cheap_comparisons == 0);
75-
assert(p == std::make_pair(m.begin() + 2, true));
76-
assert(std::ranges::equal(m, expected));
77-
}
78-
{
79-
// insert(const_iterator, P&&)
80-
M m = {1, 2, 4, 5};
81-
expensive_comparisons = 0;
82-
cheap_comparisons = 0;
83-
std::same_as<typename M::iterator> auto it = m.insert(m.begin(), 3);
84-
assert(expensive_comparisons >= 2);
85-
assert(cheap_comparisons == 0);
86-
assert(it == m.begin() + 2);
87-
assert(std::ranges::equal(m, expected));
88-
}
89-
{
90-
// insert(value_type&&)
91-
M m = {1, 2, 4, 5};
92-
expensive_comparisons = 0;
93-
cheap_comparisons = 0;
94-
std::same_as<std::pair<typename M::iterator, bool>> auto p = m.insert(3); // conversion happens last
95-
assert(expensive_comparisons >= 2);
96-
assert(cheap_comparisons == 0);
97-
assert(p == std::make_pair(m.begin() + 2, true));
98-
assert(std::ranges::equal(m, expected));
99-
}
100-
{
101-
// insert(const_iterator, value_type&&)
102-
M m = {1, 2, 4, 5};
103-
expensive_comparisons = 0;
104-
cheap_comparisons = 0;
105-
std::same_as<typename M::iterator> auto it = m.insert(m.begin(), 3);
106-
assert(expensive_comparisons >= 2);
107-
assert(cheap_comparisons == 0);
108-
assert(it == m.begin() + 2);
109-
assert(std::ranges::equal(m, expected));
110-
}
111-
{
112-
// emplace(Args&&...)
113-
M m = {1, 2, 4, 5};
114-
expensive_comparisons = 0;
115-
cheap_comparisons = 0;
116-
std::same_as<std::pair<typename M::iterator, bool>> auto p = m.emplace(3); // conversion happens first
117-
assert(expensive_comparisons >= 2);
118-
assert(cheap_comparisons == 0);
119-
assert(p == std::make_pair(m.begin() + 2, true));
120-
assert(std::ranges::equal(m, expected));
54+
const int expected[] = {1, 2, 3, 4, 5};
55+
56+
{
57+
// insert(K&&)
58+
bool transparent_used = false;
59+
M m{{1, 2, 4, 5}, TransparentComparator{transparent_used}};
60+
assert(!transparent_used);
61+
std::same_as<std::pair<typename M::iterator, bool>> decltype(auto) r =
62+
m.insert(ExplicitlyConvertibleTransparent<Key>{3});
63+
assert(transparent_used);
64+
assert(r.first == m.begin() + 2);
65+
assert(r.second);
66+
assert(std::ranges::equal(m, expected));
67+
}
68+
{
69+
// insert(const_iterator, K&&)
70+
bool transparent_used = false;
71+
M m{{1, 2, 4, 5}, TransparentComparator{transparent_used}};
72+
assert(!transparent_used);
73+
std::same_as<typename M::iterator> auto it = m.insert(m.begin(), ExplicitlyConvertibleTransparent<Key>{3});
74+
assert(transparent_used);
75+
assert(it == m.begin() + 2);
76+
assert(std::ranges::equal(m, expected));
77+
}
12178
}
79+
12280
{
12381
// was empty
124-
M m;
125-
std::same_as<std::pair<typename M::iterator, bool>> auto p = m.insert(3);
126-
assert(p == std::make_pair(m.begin(), true));
127-
M expected2 = {3};
128-
assert(std::ranges::equal(m, expected2));
82+
const int expected[] = {3};
83+
{
84+
// insert(K&&)
85+
bool transparent_used = false;
86+
M m{{}, TransparentComparator{transparent_used}};
87+
assert(!transparent_used);
88+
std::same_as<std::pair<typename M::iterator, bool>> decltype(auto) r =
89+
m.insert(ExplicitlyConvertibleTransparent<Key>{3});
90+
assert(!transparent_used); // no elements to compare against
91+
assert(r.first == m.begin());
92+
assert(r.second);
93+
assert(std::ranges::equal(m, expected));
94+
}
95+
{
96+
// insert(const_iterator, K&&)
97+
bool transparent_used = false;
98+
M m{{}, TransparentComparator{transparent_used}};
99+
assert(!transparent_used);
100+
std::same_as<typename M::iterator> auto it = m.insert(m.begin(), ExplicitlyConvertibleTransparent<Key>{3});
101+
assert(!transparent_used); // no elements to compare against
102+
assert(it == m.begin());
103+
assert(std::ranges::equal(m, expected));
104+
}
129105
}
130106
}
131107

132108
void test() {
133-
test_one<std::vector<CompareCounter>>();
134-
test_one<std::deque<CompareCounter>>();
135-
test_one<MinSequenceContainer<CompareCounter>>();
136-
test_one<std::vector<CompareCounter, min_allocator<CompareCounter>>>();
109+
test_one<std::vector<int>>();
110+
test_one<std::deque<int>>();
111+
test_one<MinSequenceContainer<int>>();
112+
test_one<std::vector<int, min_allocator<int>>>();
137113

138114
{
139115
// no ambiguity between insert(pos, P&&) and insert(first, last)
@@ -153,26 +129,14 @@ void test_exception() {
153129
{
154130
auto insert_func = [](auto& m, auto key_arg) {
155131
using FlatSet = std::decay_t<decltype(m)>;
156-
struct T {
157-
typename FlatSet::key_type key_;
158-
T(typename FlatSet::key_type key) : key_(key) {}
159-
operator typename FlatSet::value_type() const { return key_; }
160-
};
161-
T t(key_arg);
162-
m.insert(t);
132+
m.insert(ExplicitlyConvertibleTransparent<typename FlatSet::key_type>{key_arg});
163133
};
164134
test_emplace_exception_guarantee(insert_func);
165135
}
166136
{
167137
auto insert_func_iter = [](auto& m, auto key_arg) {
168138
using FlatSet = std::decay_t<decltype(m)>;
169-
struct T {
170-
typename FlatSet::key_type key_;
171-
T(typename FlatSet::key_type key) : key_(key) {}
172-
operator typename FlatSet::value_type() const { return key_; }
173-
};
174-
T t(key_arg);
175-
m.insert(m.begin(), t);
139+
m.insert(m.begin(), ExplicitlyConvertibleTransparent<typename FlatSet::key_type>{key_arg});
176140
};
177141
test_emplace_exception_guarantee(insert_func_iter);
178142
}

libcxx/test/std/containers/container.adaptors/flat.set/helpers.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,15 +64,15 @@ template <class T, bool ConvertibleToT = false>
6464
struct Transparent {
6565
T t;
6666

67-
operator T() const
67+
explicit operator T() const
6868
requires ConvertibleToT
6969
{
7070
return t;
7171
}
7272
};
7373

7474
template <class T>
75-
using ConvertibleTransparent = Transparent<T, true>;
75+
using ExplicitlyConvertibleTransparent = Transparent<T, true>;
7676

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

0 commit comments

Comments
 (0)