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.

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

🛠️ for flat_set #4050

Merged
merged 11 commits into from
Sep 27, 2023
Merged

🛠️ for flat_set #4050

Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
536c00d
1. nitpicks
achabense Sep 24, 2023
2cb6462
2. in `insert_range`, fall back to standard definition if the contain…
achabense Sep 24, 2023
a251a3e
3. though not necessary for correctness, add `_Different_from` to let…
achabense Sep 25, 2023
bf6b06a
4.1. update debug methods
achabense Sep 25, 2023
cfde52b
4.2. remove `_Keys_equal` method.
achabense Sep 25, 2023
d7e8768
5. add test coverages
achabense Sep 25, 2023
4ca5b9b
6. fix for `test_comparer_application`
achabense Sep 25, 2023
b9be818
refine tests
achabense Sep 26, 2023
fa6c030
7. update scope guard; works towards exception-conformance
achabense Sep 26, 2023
9f2e612
nitpick
achabense Sep 26, 2023
6bcbbe6
Code review feedback.
StephanTLavavej Sep 27, 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
34 changes: 21 additions & 13 deletions stl/inc/flat_set
View file
Open in desktop
Original file line number Diff line number Diff line change
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 •
edited
Loading

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 •
edited
Loading

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 Down Expand Up @@ -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 @@ -730,7 +738,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 +747,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