Skip to content

<deque>: shrink_to_fit() should follow the Standard #4072

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

Closed
StephanTLavavej opened this issue Oct 6, 2023 · 1 comment · Fixed by #4091
Closed

<deque>: shrink_to_fit() should follow the Standard #4072

StephanTLavavej opened this issue Oct 6, 2023 · 1 comment · Fixed by #4091
Labels
bug Something isn't working fixed Something works now, yay!

Comments

@StephanTLavavej
Copy link
Member

WG21-N4958 [deque.capacity]/6:

Effects: shrink_to_fit is a non-binding request to reduce memory use but does not change the size of the sequence.
[Note 1: The request is non-binding to allow latitude for implementation-specific optimizations. - end note]
If the size is equal to the old capacity, or if an exception is thrown other than by the move constructor of a non-Cpp17CopyInsertable T, then there are no effects."

We unconditionally move, which is a bug:

deque _Tmp(_STD make_move_iterator(begin()), _STD make_move_iterator(end()));

(#4071 is patching this line in an unrelated way.)

vector provides its strong guarantees correctly, like this (note that it doesn't directly use move_if_noexcept()):

STL/stl/inc/vector

Lines 837 to 842 in 283cf32

if (_Whereptr == _Mylast) { // at back, provide strong guarantee
if constexpr (is_nothrow_move_constructible_v<_Ty> || !is_copy_constructible_v<_Ty>) {
_Uninitialized_move(_Myfirst, _Mylast, _Newvec, _Al);
} else {
_Uninitialized_copy(_Myfirst, _Mylast, _Newvec, _Al);
}

@StephanTLavavej StephanTLavavej added the bug Something isn't working label Oct 6, 2023
@frederick-vs-ja
Copy link
Contributor

Reallocation of elements doesn't seem valuable since the current block size of deque is unwisely small. I think we should just deallocate unused blocks and shrink the internal map.

@StephanTLavavej StephanTLavavej changed the title <deque>: shrink_to_fit() should have move_if_noexcept() logic <deque>: shrink_to_fit() should follow the Standard Jan 26, 2024
@StephanTLavavej StephanTLavavej added the fixed Something works now, yay! label Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed Something works now, yay!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants