Skip to content

<flat_set>: flat_(multi)set's iterator type should be constant iterator #4132

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
Stephan T. Lavavej (StephanTLavavej) merged 7 commits into from
Nov 7, 2023
Merged

<flat_set>: flat_(multi)set's iterator type should be constant iterator #4132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c15cecb
1. add test for set requirements
achabense Oct 27, 2023
39eee7b
2. fixes
achabense Oct 27, 2023
a32d2b7
3. cleanups etc.
achabense Oct 27, 2023
2f1e881
Apply suggestions from code review
achabense Oct 27, 2023
cda15c2
Apply suggestions from code review 2
achabense Oct 27, 2023
150359d
review feedback: remove redundant non-const overloads.
achabense Oct 30, 2023
7189ef9
review feedback: make _Get_container non-const
achabense Oct 30, 2023
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
128 changes: 35 additions & 93 deletions stl/inc/flat_set
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,10 @@ public:
using const_reference = const value_type&;
using size_type = _Container::size_type;
using difference_type = _Container::difference_type;
using iterator = _Container::iterator;
using const_iterator = _Container::const_iterator;
using reverse_iterator = _STD reverse_iterator<iterator>;
using iterator = const_iterator;
using const_reverse_iterator = _STD reverse_iterator<const_iterator>;
using reverse_iterator = const_reverse_iterator;
using container_type = _Container;

static_assert(random_access_iterator<iterator>, "The C++ Standard forbids containers without random "
Expand Down Expand Up @@ -160,7 +160,7 @@ public:
{}

_Base_flat_set& operator=(const _Base_flat_set& _Other) {
_Clear_guard<_Base_flat_set> _Guard{this};
_Clear_guard<container_type> _Guard{_STD addressof(_Mycont)};
_Mycont = _Other._Mycont;
_Mycomp = _Other._Mycomp;
_Guard._Target = nullptr;
Expand All @@ -170,8 +170,8 @@ public:
is_nothrow_move_assignable_v<container_type>&& is_nothrow_copy_assignable_v<key_compare>) // strengthened
{
if (this != _STD addressof(_Other)) {
_Clear_guard<_Base_flat_set> _Guard{this};
_Clear_guard<_Base_flat_set> _Always_clear{_STD addressof(_Other)};
_Clear_guard<container_type> _Guard{_STD addressof(_Mycont)};
_Clear_guard<container_type> _Always_clear{_STD addressof(_Other._Mycont)};
_Mycont = _STD move(_Other._Mycont);
_Mycomp = _Other._Mycomp; // intentionally copy comparator, see LWG-2227
_Guard._Target = nullptr;
Expand All @@ -180,36 +180,25 @@ public:
}

_Deriv& operator=(initializer_list<_Kty> _Ilist) {
_Clear_guard<_Base_flat_set> _Guard{this};
_Clear_guard<container_type> _Guard{_STD addressof(_Mycont)};
_Mycont.assign(_Ilist);
_Make_invariants_fulfilled();
_Guard._Target = nullptr;
return static_cast<_Deriv&>(*this);
}

// iterators
_NODISCARD iterator begin() noexcept {
return _Mycont.begin();
}
// NB: The non-const overloads are intentionally removed for brevity. This will not result in behavioral changes.
_NODISCARD const_iterator begin() const noexcept {
return _Mycont.begin();
}
_NODISCARD iterator end() noexcept {
return _Mycont.end();
}
_NODISCARD const_iterator end() const noexcept {
return _Mycont.end();
}

_NODISCARD reverse_iterator rbegin() noexcept {
return reverse_iterator(end());
}
_NODISCARD const_reverse_iterator rbegin() const noexcept {
return const_reverse_iterator(end());
}
_NODISCARD reverse_iterator rend() noexcept {
return reverse_iterator(begin());
}
_NODISCARD const_reverse_iterator rend() const noexcept {
return const_reverse_iterator(begin());
}
Expand Down Expand Up @@ -314,28 +303,25 @@ public:
_NODISCARD container_type extract() && noexcept(
is_nothrow_move_constructible_v<container_type>) /* strengthened */ {
// always clears the container (N4950 [flat.set.modifiers]/14 and [flat.multiset.modifiers]/10)
_Clear_guard<_Base_flat_set> _Always_clear{this};
_Clear_guard<container_type> _Always_clear{_STD addressof(_Mycont)};
return _STD move(_Mycont);
}
void replace(container_type&& _Cont) {
_STL_ASSERT(_Is_sorted(_Cont), _Msg_not_sorted);
_Clear_guard<_Base_flat_set> _Guard{this};
_Clear_guard<container_type> _Guard{_STD addressof(_Mycont)};
_Mycont = _STD move(_Cont);
_Guard._Target = nullptr;
}

iterator erase(iterator _Where) {
return _Mycont.erase(_Where);
}
// NB: `erase(iterator)` is identical to `erase(const_iterator)`
iterator erase(const_iterator _Where) {
return _Mycont.erase(_Where);
}
size_type erase(const _Kty& _Val) {
return _Erase(_Val);
}
template <_Different_from<_Kty> _Other>
requires (
_Keylt_transparent && !is_convertible_v<_Other, iterator> && !is_convertible_v<_Other, const_iterator>)
requires (_Keylt_transparent && !is_convertible_v<_Other, const_iterator>)

@achabense achabense Oct 30, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is it ok not to add comment for this one? Or am I adding too many repetitive comments?

(Also, there are some remaining iterator in return types (erase etc). I find it trickey to decide whether to return const_iterator/iterator. Returning iterator is more obviously standard-conformant, but that also hides some informations...)

@StephanTLavavej Stephan T. Lavavej (StephanTLavavej) Nov 7, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I probably would have commented this, but it's not really more confusing than the other locations which silently assume that they're the same type. I think this is fine to merge as-is.

size_type erase(_Other&& _Val) {
return _Erase(_Val);
}
Expand All @@ -359,17 +345,13 @@ public:
return _Mycomp;
}

// set operations
_NODISCARD iterator find(const _Kty& _Val) {
return _Find(_Val);
}
_NODISCARD const_iterator find(const _Kty& _Val) const {
return _Find(_Val);
_NODISCARD container_type& _Get_container_for_erase_if() noexcept {
return _Mycont;
}

template <class _Other>
requires _Keylt_transparent
_NODISCARD iterator find(const _Other& _Val) {
// set operations
// NB: The non-const overloads are intentionally removed for brevity. This will not result in behavioral changes.
_NODISCARD const_iterator find(const _Kty& _Val) const {
return _Find(_Val);
}
template <class _Other>
Expand Down Expand Up @@ -402,54 +384,27 @@ public:
return _STD binary_search(cbegin(), cend(), _Val, _Pass_comp());
}

_NODISCARD iterator lower_bound(const _Kty& _Val) {
return _STD lower_bound(begin(), end(), _Val, _Pass_comp());
}
_NODISCARD const_iterator lower_bound(const _Kty& _Val) const {
return _STD lower_bound(cbegin(), cend(), _Val, _Pass_comp());
}

template <class _Other>
requires _Keylt_transparent
_NODISCARD iterator lower_bound(const _Other& _Val) {
return _STD lower_bound(begin(), end(), _Val, _Pass_comp());
}
template <class _Other>
requires _Keylt_transparent
_NODISCARD const_iterator lower_bound(const _Other& _Val) const {
return _STD lower_bound(cbegin(), cend(), _Val, _Pass_comp());
}

_NODISCARD iterator upper_bound(const _Kty& _Val) {
return _STD upper_bound(begin(), end(), _Val, _Pass_comp());
}
_NODISCARD const_iterator upper_bound(const _Kty& _Val) const {
return _STD upper_bound(cbegin(), cend(), _Val, _Pass_comp());
}

template <class _Other>
requires _Keylt_transparent
_NODISCARD iterator upper_bound(const _Other& _Val) {
return _STD upper_bound(begin(), end(), _Val, _Pass_comp());
}
template <class _Other>
requires _Keylt_transparent
_NODISCARD const_iterator upper_bound(const _Other& _Val) const {
return _STD upper_bound(cbegin(), cend(), _Val, _Pass_comp());
}

_NODISCARD pair<iterator, iterator> equal_range(const _Kty& _Val) {
return _STD equal_range(begin(), end(), _Val, _Pass_comp());
}
_NODISCARD pair<const_iterator, const_iterator> equal_range(const _Kty& _Val) const {
return _STD equal_range(cbegin(), cend(), _Val, _Pass_comp());
}

template <class _Other>
requires _Keylt_transparent
_NODISCARD pair<iterator, iterator> equal_range(const _Other& _Val) {
return _STD equal_range(begin(), end(), _Val, _Pass_comp());
}
template <class _Other>
requires _Keylt_transparent
_NODISCARD pair<const_iterator, const_iterator> equal_range(const _Other& _Val) const {
Expand Down Expand Up @@ -521,9 +476,8 @@ private:
template <class _Ty>
requires (!_Multi) // flat_set
_NODISCARD pair<iterator, bool> _Emplace(_Ty&& _Val) {
const iterator _Where = lower_bound(_Val);
if (_Where != end() && !_Compare(_Val, *_Where)) {
// *_Where is equivalent to _Val.
const const_iterator _Where = lower_bound(_Val);
if (_Where != cend() && !_Compare(_Val, *_Where)) {
return pair{_Where, false};
}

Expand Down Expand Up @@ -589,8 +543,7 @@ private:
}

if (_Where != _End && !_Compare(_Val, *_Where)) {
// *_Where is equivalent to _Val; convert _Where to iterator type.
return _Mycont.begin() + (_Where - _Begin);
return _Where;
}

if constexpr (is_same_v<remove_cvref_t<_Ty>, _Kty>) {
Expand Down Expand Up @@ -620,8 +573,8 @@ private:
_STL_INTERNAL_STATIC_ASSERT(_Keylt_transparent || is_same_v<_Ty, _Kty>);

if constexpr (!_Multi && is_same_v<_Ty, _Kty>) {
const iterator _Where = lower_bound(_Val);
if (_Where != end() && !_Compare(_Val, *_Where)) {
const const_iterator _Where = lower_bound(_Val);
if (_Where != cend() && !_Compare(_Val, *_Where)) {
_Mycont.erase(_Where);
return 1;
}
Expand All @@ -635,19 +588,6 @@ private:
}
}

template <class _Ty>
_NODISCARD iterator _Find(const _Ty& _Val) {
_STL_INTERNAL_STATIC_ASSERT(_Keylt_transparent || is_same_v<_Ty, _Kty>);

const iterator _End = end();
const iterator _Where = lower_bound(_Val);
if (_Where != _End && !_Compare(_Val, *_Where)) {
return _Where;
} else {
return _End;
}
}

template <class _Ty>
_NODISCARD const_iterator _Find(const _Ty& _Val) const {
_STL_INTERNAL_STATIC_ASSERT(_Keylt_transparent || is_same_v<_Ty, _Kty>);
Expand All @@ -666,16 +606,16 @@ private:
const auto _Equivalent = [this](const _Kty& _Lhs, const _Kty& _Rhs) {
return !_Compare(_Lhs, _Rhs) && !_Compare(_Rhs, _Lhs);
};
const iterator _End = end();
_Mycont.erase(_STD unique(begin(), _End, _Equivalent), _End);
const auto _End = _Mycont.end();
_Mycont.erase(_STD unique(_Mycont.begin(), _End, _Equivalent), _End);
}
}

template <bool _Presorted>
void _Restore_invariants_after_insert(const size_type _Old_size) {
const iterator _Begin = begin();
const iterator _Old_end = _Begin + static_cast<difference_type>(_Old_size);
const iterator _End = end();
const auto _Begin = _Mycont.begin();
const auto _Old_end = _Begin + static_cast<difference_type>(_Old_size);
const auto _End = _Mycont.end();

if constexpr (!_Presorted) {
_STD sort(_Old_end, _End, _Pass_comp());
Expand All @@ -690,15 +630,15 @@ private:
}

void _Make_invariants_fulfilled() {
const iterator _Begin = begin();
const iterator _End = end();
const auto _Begin = _Mycont.begin();
const auto _End = _Mycont.end();

if (_Begin == _End) {
return;
}

// O(N) if already sorted.
const iterator _Begin_unsorted = _STD is_sorted_until(_Begin, _End, _Pass_comp());
const auto _Begin_unsorted = _STD is_sorted_until(_Begin, _End, _Pass_comp());

_STD sort(_Begin_unsorted, _End, _Pass_comp());
_STD inplace_merge(_Begin, _Begin_unsorted, _End, _Pass_comp());
Expand Down Expand Up @@ -768,17 +708,19 @@ public:
_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_guard<flat_set<_Kty, _Keylt, _Container>> _Guard{_STD addressof(_Val)};
const auto _Erased_count = _STD _Erase_remove_if(_Val, _STD _Pass_fn(_Predicate));
_Container& _Cont = _Val._Get_container_for_erase_if();
_Clear_guard<_Container> _Guard{_STD addressof(_Cont)};
const auto _Erased_count = _STD _Erase_remove_if(_Cont, _STD _Pass_fn(_Predicate));
_Guard._Target = nullptr;
return _Erased_count;
}

_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_guard<flat_multiset<_Kty, _Keylt, _Container>> _Guard{_STD addressof(_Val)};
const auto _Erased_count = _STD _Erase_remove_if(_Val, _STD _Pass_fn(_Predicate));
_Container& _Cont = _Val._Get_container_for_erase_if();
_Clear_guard<_Container> _Guard{_STD addressof(_Cont)};
const auto _Erased_count = _STD _Erase_remove_if(_Cont, _STD _Pass_fn(_Predicate));
_Guard._Target = nullptr;
return _Erased_count;
}
Expand Down
19 changes: 18 additions & 1 deletion tests/std/tests/P1222R4_flat_set/test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ void assert_container_requirements(const T& s) {
static_assert(is_same_v<decltype(m.begin() <=> m.end()), strong_ordering>);
static_assert(is_same_v<decltype(s.size()), typename T::size_type>);
static_assert(is_same_v<decltype(s.max_size()), typename T::size_type>);
static_assert(is_same_v<decltype(*m.begin()), typename T::value_type&>);
static_assert(is_same_v<decltype(*m.begin()), const typename T::value_type&>);
static_assert(is_same_v<decltype(*m.cbegin()), const typename T::value_type&>);

T my_moved = std::move(m);
Expand Down Expand Up @@ -73,6 +73,22 @@ void assert_reversible_container_requirements(const T& s) {
static_assert(is_convertible_v<typename T::reverse_iterator, typename T::const_reverse_iterator>);
}

template <class T>
void assert_set_requirements() {
using iterator = T::iterator;
using const_iterator = T::const_iterator;
using key_type = T::key_type;
using value_type = T::value_type;

static_assert(same_as<std::const_iterator<const_iterator>, const_iterator>);
static_assert(is_convertible_v<iterator, const_iterator>);

// additionally:
static_assert(is_same_v<key_type, value_type>);
static_assert(same_as<std::const_iterator<iterator>, iterator>);
static_assert(is_convertible_v<const_iterator, iterator>);
}

template <class T>
void assert_noexcept_requirements(T& s) {
static_assert(noexcept(s.begin()));
Expand All @@ -99,6 +115,7 @@ template <class T>
void assert_all_requirements(const T& s) {
assert_container_requirements(s);
assert_reversible_container_requirements(s);
assert_set_requirements<T>();

assert_noexcept_requirements(s);
assert_noexcept_requirements(const_cast<T&>(s));
Expand Down