Skip to content
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

🛠️ for flat_set #4050

Merged
merged 11 commits into from
Sep 27, 2023
105 changes: 51 additions & 54 deletions stl/inc/flat_set
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ public:

_Base_flat_set(_Tsorted, container_type _Cont, const key_compare& _Comp = key_compare())
: _My_pair(_One_then_variadic_args_t{}, _Comp, _STD move(_Cont)) {
_Assert_after_sorted_input();
_STL_ASSERT(_Check_sorted(cbegin(), cend()), _Msg_not_sorted);
}
template <_Allocator_for<container_type> _Alloc>
_Base_flat_set(_Tsorted _Tsort, const container_type& _Cont, const _Alloc& _Al)
Expand Down Expand Up @@ -228,25 +228,25 @@ public:
}
}

auto insert(const value_type& _Val) {
auto insert(const _Kty& _Val) {
return _Emplace(_Val);
}
auto insert(value_type&& _Val) {
auto insert(_Kty&& _Val) {
achabense marked this conversation as resolved.
Show resolved Hide resolved
return _Emplace(_STD move(_Val));
}
template <class _Other>
template <_Different_from<_Kty> _Other>
Copy link
Contributor Author

@achabense achabense Sep 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, without this constraint, when the comparer is transparent inputs like non-const _Kty& will go into this function. The case will be correctly handled by _Emplace, but I think it will be better just fall to insert(const _Kty&).

requires (!_Multi && _Keylt_transparent && is_constructible_v<_Kty, _Other>)
auto insert(_Other&& _Val) {
return _Emplace(_STD forward<_Other>(_Val));
}

iterator insert(const_iterator _Hint, const value_type& _Val) {
iterator insert(const_iterator _Hint, const _Kty& _Val) {
return _Emplace_hint(_Hint, _Val);
}
iterator insert(const_iterator _Hint, value_type&& _Val) {
iterator insert(const_iterator _Hint, _Kty&& _Val) {
return _Emplace_hint(_Hint, _STD move(_Val));
}
template <class _Other>
template <_Different_from<_Kty> _Other>
requires (!_Multi && _Keylt_transparent && is_constructible_v<_Kty, _Other>)
iterator insert(const_iterator _Hint, _Other&& _Val) {
return _Emplace_hint(_Hint, _STD forward<_Other>(_Val));
Expand All @@ -263,7 +263,15 @@ public:
template <_Container_compatible_range<_Kty> _Rng>
void insert_range(_Rng&& _Range) {
const size_type _Old_size = size();
_Get_cont().append_range(_STD forward<_Rng>(_Range));

_Container& _Cont = _Get_cont();
if constexpr (requires { _Cont.append_range(_STD forward<_Rng>(_Range)); }) {
_Cont.append_range(_STD forward<_Rng>(_Range));
Copy link
Contributor Author

@achabense achabense Sep 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

append_range should be considered an optimization here.
I notice that insert_range(end,...) (instead of append_range) is unconditionally required for sequence containers. However, I think we don't have to give an extra check for that chance, as it is likely that a container that doesn't support append_range will not support insert_range either.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I find these funtions lack forward (required by the standard):

STL/stl/inc/stack

Lines 115 to 117 in 48eedd3

void push_range(_Rng&& _Range) {
if constexpr (requires { c.append_range(_Range); }) {
c.append_range(_Range);

STL/stl/inc/queue

Lines 125 to 127 in 48eedd3

void push_range(_Rng&& _Range) {
if constexpr (requires { c.append_range(_Range); }) {
c.append_range(_Range);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we forward here? It seems that std::forward merely rejects non-borrowed-range rvalues, which defends dangling.

But we are eagerly consuming ranges here, so it seems to me that we can just use the ranges as lvalues for containers. CC-ing @cor3ntin @ericniebler @CaseyCarter.

} else {
for (const auto& _Val : _Range) {
_Cont.insert(_Cont.end(), _Val);
}
}
_Restore_invariants_after_insert<false>(_Old_size);
}

Expand All @@ -280,8 +288,8 @@ public:
return _STD move(_Get_cont());
}
void replace(container_type&& _Cont) {
_STL_ASSERT(_Check_sorted(_Cont.cbegin(), _Cont.cend()), _Msg_not_sorted);
Copy link
Contributor Author

@achabense achabense Sep 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd really hope to add noexcept enhancement for replace, but I have no idea how to deal with this _STL_ASSERT :(

_Get_cont() = _STD move(_Cont);
_Assert_after_sorted_input();
}

iterator erase(iterator _Where) {
Expand All @@ -293,7 +301,7 @@ public:
size_type erase(const _Kty& _Val) {
return _Erase(_Val);
}
template <class _Other>
template <_Different_from<_Kty> _Other>
requires (
_Keylt_transparent && !is_convertible_v<_Other, iterator> && !is_convertible_v<_Other, const_iterator>)
size_type erase(_Other&& _Val) {
Expand Down Expand Up @@ -339,11 +347,11 @@ public:
}

_NODISCARD size_type count(const _Kty& _Val) const {
if constexpr (!_Multi) {
return contains(_Val);
} else {
if constexpr (_Multi) {
const auto [_First, _Last] = equal_range(_Val);
return static_cast<size_type>(_Last - _First);
} else {
return contains(_Val);
}
}
template <class _Other>
Expand Down Expand Up @@ -420,7 +428,7 @@ public:
return _RANGES equal(_Lhs._Get_cont(), _Rhs._Get_cont());
}

_NODISCARD friend _Synth_three_way_result<_Kty> operator<=>(const _Deriv& _Lhs, const _Deriv& _Rhs) {
_NODISCARD friend auto operator<=>(const _Deriv& _Lhs, const _Deriv& _Rhs) {
Copy link
Contributor Author

@achabense achabense Sep 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was making test_comparer_application fail to compile.

... though this "fixes" the problem, I'm afraid we need to look into what is actually going on around this.

This comment was marked as outdated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe not a compiler bug, the code is rejected by both msvc, gcc and clang :|
https://godbolt.org/z/jT6sT4EfT (the error message given by msvc is the strangest one though)

#include<utility>

#define USE_FRIEND

struct myless {
    template<class L, class R>
    auto operator()(const L& lhs, const R& rhs) {
        return lhs < rhs;
    }
};

template<class L, class R = L>
using myless_result = decltype(myless{}(std::declval<L&>(), std::declval<R&>()));

template<class T>
struct wrapper {
    T t;
#if defined USE_FRIEND
#if 1
    // "unexpected token(s) preceding '{'; skipping apparent function body"
    // then other errors.
    friend myless_result<T> operator<(const wrapper& lhs, const wrapper& rhs) {
        return myless{}(lhs.t, rhs.t);
    }
#else
    // will be ok if returns auto.
    friend auto operator<(const wrapper& lhs, const wrapper& rhs) {
        return myless{}(lhs.t, rhs.t);
    }
#endif

#endif // USE_FRIEND
};

#if !defined USE_FRIEND
// will be ok if not friend.
template<class T>
myless_result<T> operator<(const wrapper<T>& lhs, const wrapper<T>& rhs) {
    return myless{}(lhs, rhs);
}
#endif // !USE_FRIEND

struct xcmp {
    bool operator=(const xcmp&)const = delete;
    bool operator<(const xcmp&)const = delete;
};

int main() {
    wrapper<xcmp> test;
}

return _STD lexicographical_compare_three_way(
_Lhs.cbegin(), _Lhs.cend(), _Rhs.cbegin(), _Rhs.cend(), _Synth_three_way{});
}
Expand All @@ -430,27 +438,25 @@ public:
}

private:
void _Assert_after_sorted_input() const {
_STL_ASSERT(_STD is_sorted(cbegin(), cend(), _Get_comp_v()), "Input was not sorted!");
if constexpr (!_Multi) {
_STL_ASSERT(_Is_unique(), "Input was sorted but not unique!");
}
}

bool _Is_unique() const {
if (empty()) {
return true;
}
const const_iterator _End = cend();
const_iterator _It = cbegin();
while (++_It != _End) {
if (_Keys_equal(*(_It - 1), *_It)) {
return false;
bool _Check_sorted(const_iterator _It, const const_iterator _End) const {
StephanTLavavej marked this conversation as resolved.
Show resolved Hide resolved
if (_Multi) {
StephanTLavavej marked this conversation as resolved.
Show resolved Hide resolved
return _STD is_sorted(_It, _End, _Get_comp_v());
} else {
// sorted-unique
if (_It == _End) {
return true;
}
while (++_It != _End) {
if (!_Compare(*(_It - 1), *_It)) {
return false;
}
}
return true;
}
return true;
}

static constexpr const char* _Msg_not_sorted = _Multi ? "Input was not sorted!" : "Input was not sorted-unique!";
Copy link
Contributor Author

@achabense achabense Sep 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea how to represent this gracefully :( It might be helpful to an additional version of _STL_ASSERT that repeat error message just like _STL_INTERNAL_CHECK and assert.

Also, these references in static_assert are not correct, as _Base_flat_set is also the base for flat_multiset.

STL/stl/inc/flat_set

Lines 47 to 49 in 6bedc29

static_assert(same_as<_Kty, typename _Container::value_type>,
"The C++ Standard dictates that the Key type must be the "
"same as the container's value type [flatset.overview]");

STL/stl/inc/flat_set

Lines 65 to 66 in 6bedc29

static_assert(random_access_iterator<iterator>, "The C++ Standard forbids containers without random "
"access iterators from being adapted. See [flatset.overview].");


bool _Check_where(const const_iterator _Where, const _Kty& _Val) const {
// check that _Val can be inserted before _Where
if constexpr (_Multi) {
Expand Down Expand Up @@ -604,17 +610,18 @@ private:
}
}

void _Erase_dupes_if_needed() {
void _Erase_dupes_if_not_multi() {
if constexpr (!_Multi) {
const iterator _End = end();
const iterator _New_end =
_STD unique(begin(), _End, [&](const _Kty& _Lhs, const _Kty& _Rhs) { return _Keys_equal(_Lhs, _Rhs); });
const auto _Equal_to = [this](const _Kty& _Lhs, const _Kty& _Rhs) {
return !_Compare(_Lhs, _Rhs) && !_Compare(_Rhs, _Lhs);
};
const iterator _End = end();
const iterator _New_end = _STD unique(begin(), _End, _Equal_to);
_Get_cont().erase(_New_end, _End);

_STL_INTERNAL_CHECK(_Is_unique());
}
}


achabense marked this conversation as resolved.
Show resolved Hide resolved
template <bool _Presorted>
void _Restore_invariants_after_insert(const size_type _Old_size) {
auto _Comp = _Get_comp_v();
Expand All @@ -625,14 +632,13 @@ private:
if constexpr (!_Presorted) {
_STD sort(_Old_end, _New_end, _Comp);
} else {
_STL_ASSERT(_STD is_sorted(_Old_end, _New_end, _Comp), "Input was not sorted!");
_STL_ASSERT(_Check_sorted(_Old_end, _New_end), _Msg_not_sorted);
achabense marked this conversation as resolved.
Show resolved Hide resolved
}

_STD inplace_merge(_Begin, _Old_end, _New_end, _Comp);
_Erase_dupes_if_not_multi();

_STL_INTERNAL_CHECK(_STD is_sorted(_Begin, _New_end, _Comp));

_Erase_dupes_if_needed();
_STL_INTERNAL_CHECK(_Check_sorted(cbegin(), cend()));
}

void _Make_invariants_fulfilled() {
Expand All @@ -649,10 +655,9 @@ private:

_STD sort(_Begin_unsorted, _End, _Comp);
_STD inplace_merge(_Begin, _Begin_unsorted, _End, _Comp);
_Erase_dupes_if_not_multi();

_STL_INTERNAL_CHECK(_STD is_sorted(_Begin, _End, _Comp));

_Erase_dupes_if_needed();
_STL_INTERNAL_CHECK(_Check_sorted(cbegin(), cend()));
}

template <class _Lty, class _Rty>
Expand All @@ -663,14 +668,6 @@ private:
return _DEBUG_LT_PRED(_My_pair._Get_first(), _Lhs, _Rhs);
}

template <class _Lty, class _Rty>
_NODISCARD bool _Keys_equal(const _Lty& _Lhs, const _Rty& _Rhs) const
noexcept(noexcept(!_Compare(_Lhs, _Rhs) && !_Compare(_Rhs, _Lhs))) {
_STL_INTERNAL_STATIC_ASSERT(_Keylt_transparent || (is_same_v<_Kty, _Lty> && is_same_v<_Kty, _Rty>) );

return !_Compare(_Lhs, _Rhs) && !_Compare(_Rhs, _Lhs);
}

_NODISCARD const _Container& _Get_cont() const noexcept {
return _My_pair._Myval2;
}
Expand Down Expand Up @@ -730,7 +727,7 @@ _EXPORT_STD template <class _Kty, class _Keylt, class _Container, class _Pred>
_Container::size_type erase_if(flat_set<_Kty, _Keylt, _Container>& _Val, _Pred _Predicate) {
// clears the container to maintain the invariants when an exception is thrown (N4950 [flat.set.erasure]/5)
_Clear_scope_guard<flat_set<_Kty, _Keylt, _Container>> _Guard{_STD addressof(_Val)};
const auto _Erased_count = _Erase_remove_if(_Val, _Pass_fn(_Predicate));
const auto _Erased_count = _STD _Erase_remove_if(_Val, _STD _Pass_fn(_Predicate));
_Guard._Clearable = nullptr;
return _Erased_count;
}
Expand All @@ -739,7 +736,7 @@ _EXPORT_STD template <class _Kty, class _Keylt, class _Container, class _Pred>
_Container::size_type erase_if(flat_multiset<_Kty, _Keylt, _Container>& _Val, _Pred _Predicate) {
// clears the container to maintain the invariants when an exception is thrown (N4950 [flat.multiset.erasure]/5)
_Clear_scope_guard<flat_multiset<_Kty, _Keylt, _Container>> _Guard{_STD addressof(_Val)};
const auto _Erased_count = _Erase_remove_if(_Val, _Pass_fn(_Predicate));
const auto _Erased_count = _STD _Erase_remove_if(_Val, _STD _Pass_fn(_Predicate));
_Guard._Clearable = nullptr;
return _Erased_count;
}
Expand Down
109 changes: 107 additions & 2 deletions tests/std/tests/P1222R4_flat_set/test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <functional>
#include <iostream>
#include <memory>
#include <random>
#include <vector>

using namespace std;
Expand Down Expand Up @@ -235,6 +236,107 @@ void test_insert_2() {
}
}

struct val_comparer {
const auto& extract_key(const auto& obj) const {
if constexpr (requires { void(obj.val); }) {
return obj.val;
} else {
return obj;
}
}

bool operator()(const auto& lhs, const auto& rhs) const {
return extract_key(lhs) < extract_key(rhs);
}

using is_transparent = int;
};

void test_comparer_application() {
achabense marked this conversation as resolved.
Show resolved Hide resolved
struct int_holder {
int val;
bool operator<(const int_holder&) const = delete;
bool operator==(const int_holder&) const = delete;
};

flat_set<int_holder, val_comparer> fs{{0}, {3}, {1}, {0}, {5}};
assert(fs.contains(0));
assert(!fs.contains(2));
fs.insert(fs.begin(), int_holder{4});
fs.insert(2);
assert(fs.contains(4));
assert(fs.contains(int_holder{2}));

assert(fs.lower_bound(3) == fs.lower_bound(int_holder{3}));
fs.erase(2);
assert(!fs.contains(int_holder{2}));
}

void test_insert_3() {
// test that flat_set::insert(K&&) doesn't modify input for failed insertion.
struct int_holder {
int val;
mutable bool converted = false;

explicit operator int() const {
converted = true;
return val;
}
};

flat_set<int, val_comparer> fs{0, 3, 5};

assert_all_requirements_and_equals(fs, {0, 3, 5});

int_holder holder{3};
assert(!holder.converted);

fs.insert(holder);
assert(!holder.converted);
assert_all_requirements_and_equals(fs, {0, 3, 5});

holder.val = 1;
fs.insert(holder);
assert(holder.converted);
assert_all_requirements_and_equals(fs, {0, 1, 3, 5});
}

void test_insert_4() {
// test that hinted insertion is robust against invalid hints.
mt19937 eng(42);

uniform_int_distribution<int> dist_seq(0, 20);

vector<int> seq(200);
for (int& val : seq) {
val = dist_seq(eng);
}

{
flat_multiset<int> with_hint, no_hint;
StephanTLavavej marked this conversation as resolved.
Show resolved Hide resolved
for (const int val : seq) {
uniform_int_distribution<int> dist_idx(0, int(with_hint.size()));
StephanTLavavej marked this conversation as resolved.
Show resolved Hide resolved
auto random_hint = with_hint.begin() + dist_idx(eng);
with_hint.insert(random_hint, val);
no_hint.insert(val);
}

assert(with_hint == no_hint);
}

{
flat_set<int> with_hint, no_hint;
for (const int val : seq) {
uniform_int_distribution<int> dist_idx(0, int(with_hint.size()));
auto random_hint = with_hint.begin() + dist_idx(eng);
with_hint.insert(random_hint, val);
no_hint.insert(val);
}

assert(with_hint == no_hint);
}
}

template <class T>
void test_spaceship_operator() {
static constexpr bool multi = _Is_specialization_v<T, flat_multiset>;
Expand Down Expand Up @@ -342,8 +444,8 @@ void test_count() {
flat_set<int> fs{2};
assert(fs.count(1) == 0);

flat_multiset<int> fs2{1, 2, 2, 3};
assert(fs2.count(2) == 2);
flat_multiset<int> fs2{10, 20, 20, 30};
assert(fs2.count(20) == 2);
}

int main() {
Expand All @@ -363,7 +465,10 @@ int main() {
test_insert_1<deque<int>>();
test_insert_2<vector<int>>();
test_insert_2<deque<int>>();
test_insert_3();
test_insert_4();

test_comparer_application();
test_non_static_comparer();

test_extract<flat_set<int>>();
Expand Down