Skip to content

More enhancements for <flat_set> #4019

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 6 commits into from
Sep 19, 2023
Merged

More enhancements for <flat_set> #4019

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d80ef87
1. the container is not necessarily reversible
achabense Sep 19, 2023
8ae785d
2. for private methods, use `static_assert` instead of constraints
achabense Sep 19, 2023
b8944b6
3. nitpicks
achabense Sep 19, 2023
53591cd
4. apply `_Pass_fn` when passing to algorithm; apply `_DEBUG_LT_PRED`…
achabense Sep 19, 2023
50000dc
5. replace some `_Keys_equal` with single comparision
achabense Sep 19, 2023
40ea038
6. optimize `flat_set.count(const key&)` and `flat_set.erase(const ke…
achabense Sep 19, 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
159 changes: 94 additions & 65 deletions stl/inc/flat_set
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,29 +172,29 @@ public:
}

_NODISCARD reverse_iterator rbegin() noexcept {
return _Get_cont().rbegin();
return reverse_iterator(end());
}
_NODISCARD const_reverse_iterator rbegin() const noexcept {
return _Get_cont().rbegin();
return const_reverse_iterator(end());
}
_NODISCARD reverse_iterator rend() noexcept {
return _Get_cont().rend();
return reverse_iterator(begin());
}
_NODISCARD const_reverse_iterator rend() const noexcept {
return _Get_cont().rend();
return const_reverse_iterator(begin());
}

_NODISCARD const_iterator cbegin() const noexcept {
return _Get_cont().cbegin();
return begin();
}
_NODISCARD const_iterator cend() const noexcept {
return _Get_cont().cend();
return end();

@achabense achabense Sep 9, 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.

As to rbegin/...: flat_meow should be reversible, while the container just have to be sequential and provide random-access iterator.
As to cbegin/end: the container should provide these methods; however I think for style consistency it's also not harmful to directly call begin/end here.

}
_NODISCARD const_reverse_iterator crbegin() const noexcept {
return _Get_cont().crbegin();
return rbegin();
}
_NODISCARD const_reverse_iterator crend() const noexcept {
return _Get_cont().crend();
return rend();
}

// capacity
Expand Down Expand Up @@ -339,8 +339,12 @@ public:
}

_NODISCARD size_type count(const _Kty& _Val) const {
const auto [_First, _Last] = equal_range(_Val);
return static_cast<size_type>(_Last - _First);
if constexpr (!_Multi) {

@StephanTLavavej Stephan T. Lavavej (StephanTLavavej) Sep 19, 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.

No change requested (not worth resetting testing): We generally prefer to avoid negated conditions, unless there's a specific reason why they're desirable. Preferring positive conditions makes code easier to read, since it avoids double negation. Here, if constexpr (_Multi) would be good.

return contains(_Val);
} else {
const auto [_First, _Last] = equal_range(_Val);
return static_cast<size_type>(_Last - _First);
}
}
template <class _Other>
requires _Keylt_transparent
Expand All @@ -350,66 +354,66 @@ public:
}

_NODISCARD bool contains(const _Kty& _Val) const {
return _STD binary_search(cbegin(), cend(), _Val, _Get_comp());
return _STD binary_search(cbegin(), cend(), _Val, _Get_comp_v());
}
template <class _Other>
requires _Keylt_transparent
_NODISCARD bool contains(const _Other& _Val) const {
return _STD binary_search(cbegin(), cend(), _Val, _Get_comp());
return _STD binary_search(cbegin(), cend(), _Val, _Get_comp_v());
}

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

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

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

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

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

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

_NODISCARD friend bool operator==(const _Deriv& _Lhs, const _Deriv& _Rhs) {
Expand All @@ -427,7 +431,7 @@ public:

private:
void _Assert_after_sorted_input() const {
_STL_ASSERT(_STD is_sorted(cbegin(), cend(), _Get_comp()), "Input was not sorted!");
_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!");
}
Expand All @@ -449,7 +453,6 @@ private:

bool _Check_where(const const_iterator _Where, const _Kty& _Val) const {
// check that _Val can be inserted before _Where
const key_compare& _Compare = _Get_comp();
if constexpr (_Multi) {
// check that _Where is the upper_bound for _Val
// equivalent to checking *(_Where-1) <= _Val < *_Where
Expand All @@ -472,7 +475,7 @@ private:
} else {
const iterator _End = end();
const iterator _Where = lower_bound(_Val);
if (_Where != _End && _Keys_equal(*_Where, _Val)) {
if (_Where != _End && !_Compare(_Val, *_Where)) {
return pair{_Where, false};
}

Expand All @@ -491,7 +494,7 @@ private:
template <class _Ty>
iterator _Emplace_hint(const_iterator _Where, _Ty&& _Val) {
_Container& _Cont = _Get_cont();
const key_compare& _Compare = _Get_comp();
auto _Comp = _Get_comp_v();
const const_iterator _Begin = cbegin();
const const_iterator _End = cend();

Expand All @@ -503,11 +506,11 @@ private:
// _Val >= *(_Where-1) ~ upper_bound is _Where
} else {
// _Val < *(_Where-1) ~ upper_bound is in [_Begin,_Where-1]
_Where = _STD upper_bound(_Begin, _Where - 1, _Val, _Compare);
_Where = _STD upper_bound(_Begin, _Where - 1, _Val, _Comp);
}
} else {
// _Val >= *_Where ~ upper_bound is in [_Where+1,_End]
_Where = _STD upper_bound(_Where + 1, _End, _Val, _Compare);
_Where = _STD upper_bound(_Where + 1, _End, _Val, _Comp);
}
} else {
// look for the lower_bound for flat_set
Expand All @@ -517,11 +520,11 @@ private:
// _Val > *(_Where-1) ~ lower_bound is _Where
} else {
// _Val <= *(_Where-1) ~ lower_bound is in [_Begin,_Where-1]
_Where = _STD lower_bound(_Begin, _Where - 1, _Val, _Compare);
_Where = _STD lower_bound(_Begin, _Where - 1, _Val, _Comp);
}
} else {
// _Val > *_Where ~ lower_bound is in [_Where+1,_End]
_Where = _STD lower_bound(_Where + 1, _End, _Val, _Compare);
_Where = _STD lower_bound(_Where + 1, _End, _Val, _Comp);
}
}

Expand All @@ -530,7 +533,7 @@ private:
_STL_INTERNAL_CHECK(_Check_where(_Where, _Val));
return _Cont.emplace(_Where, _STD forward<_Ty>(_Val));
} else {
if (_Where != _End && _Keys_equal(_Val, *_Where)) {
if (_Where != _End && !_Compare(_Val, *_Where)) {
return _Cont.begin() + (_Where - _Begin);
}

Expand All @@ -556,46 +559,51 @@ private:
}

template <class _Ty>
requires _Keylt_transparent || is_same_v<_Ty, _Kty>
size_type _Erase(const _Ty& _Val) {
const auto [_First, _Last] = equal_range(_Val);
_STL_INTERNAL_STATIC_ASSERT(_Keylt_transparent || is_same_v<_Ty, _Kty>);

const auto _Removed = static_cast<size_type>(_Last - _First);
_Get_cont().erase(_First, _Last);
return _Removed;
if constexpr (!_Multi && is_same_v<_Ty, _Kty>) {
const iterator _Where = lower_bound(_Val);
if (_Where != end() && !_Compare(_Val, *_Where)) {
_Get_cont().erase(_Where);
return 1;
}
return 0;
} else {
const auto [_First, _Last] = equal_range(_Val);

const auto _Removed = static_cast<size_type>(_Last - _First);
_Get_cont().erase(_First, _Last);
return _Removed;
}
}

template <class _Other>
requires _Keylt_transparent || is_same_v<_Other, _Kty>
_NODISCARD iterator _Find(const _Other& _Val) {
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 && _Keys_equal(*_Where, _Val)) {
if (_Where != _End && !_Compare(_Val, *_Where)) {
return _Where;
} else {
return _End;
}
}

template <class _Other>

@achabense achabense Sep 9, 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.

I think it's ok to rename to _Ty as the type includes both key_type and other types, and _Emplace etc uses _Ty too.

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

const const_iterator _End = cend();
const const_iterator _Where = lower_bound(_Val);
if (_Where != _End && _Keys_equal(*_Where, _Val)) {
if (_Where != _End && !_Compare(_Val, *_Where)) {
return _Where;
} else {
return _End;
}
}

template <class _Lhty, class _Rhty>
requires _Keylt_transparent || (is_same_v<_Kty, _Lhty> && is_same_v<_Lhty, _Rhty>)
_NODISCARD bool _Keys_equal(const _Lhty& _Lhs, const _Rhty& _Rhs) const {
const key_compare& _Compare = _Get_comp();
return !_Compare(_Lhs, _Rhs) && !_Compare(_Rhs, _Lhs);
}

void _Erase_dupes_if_needed() {
if constexpr (!_Multi) {
const iterator _End = end();
Expand All @@ -609,19 +617,20 @@ private:

template <bool _Presorted>
void _Restore_invariants_after_insert(const size_type _Old_size) {
const key_compare& _Compare = _Get_comp();
const iterator _Old_end = begin() + static_cast<difference_type>(_Old_size);
const iterator _New_end = end();
auto _Comp = _Get_comp_v();
const iterator _Begin = begin();
const iterator _Old_end = _Begin + static_cast<difference_type>(_Old_size);
const iterator _New_end = end();

if constexpr (!_Presorted) {
_STD sort(_Old_end, _New_end, _Compare);
_STD sort(_Old_end, _New_end, _Comp);
} else {
_STL_ASSERT(_STD is_sorted(_Old_end, _New_end, _Compare), "Input was not sorted!");
_STL_ASSERT(_STD is_sorted(_Old_end, _New_end, _Comp), "Input was not sorted!");
}

_STD inplace_merge(begin(), _Old_end, _New_end, _Compare);
_STD inplace_merge(_Begin, _Old_end, _New_end, _Comp);

_STL_INTERNAL_CHECK(_STD is_sorted(begin(), end(), _Compare));
_STL_INTERNAL_CHECK(_STD is_sorted(_Begin, _New_end, _Comp));

_Erase_dupes_if_needed();
}
Expand All @@ -635,17 +644,33 @@ private:
}

// O(N) if already sorted.
const key_compare& _Compare = _Get_comp();
const iterator _Begin_unsorted = _STD is_sorted_until(_Begin, _End, _Compare);
auto _Comp = _Get_comp_v();
const iterator _Begin_unsorted = _STD is_sorted_until(_Begin, _End, _Comp);

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

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

_Erase_dupes_if_needed();
}

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

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 All @@ -662,6 +687,10 @@ private:
return _My_pair._Get_first();
}

_NODISCARD auto _Get_comp_v() const noexcept {
return _STD _Pass_fn(_My_pair._Get_first());
}

@achabense achabense Sep 11, 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.

the meaning of _v looks not very obvious (suitable for passing by value / passed through _Pass_fn); however I have difficulty finding a more suitable name here...

@frederick-vs-ja A. Jiang (frederick-vs-ja) Sep 12, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How about _Get_comp_for_passing? Although it's a bit lengthy.

@achabense achabense Sep 12, 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.

Oh, what about _Pass_comp? It seems unnecessary to keep the _Get_comp pefix 💫

_Compressed_pair<key_compare, container_type> _My_pair;
};

Expand Down