From 841132efda2157c5f9e07cf31469470a6481ffd9 Mon Sep 17 00:00:00 2001 From: Marek Kurdej Date: Thu, 26 Nov 2020 10:07:16 +0100 Subject: [PATCH] [libc++] [P0966] [C++20] Fix bug PR45368 by correctly implementing P0966: string::reserve should not shrink. This patch fixes the implementation as well as the tests that didn't actually test the wanted behaviour. You'll find all the details in the bug report. It adds as well deprecation warning for reserve() (without argument) and adds a test. http://wg21.link/P0966R1 https://bugs.llvm.org/show_bug.cgi?id=45368 https://reviews.llvm.org/D54992 Reviewed By: ldionne, #libc Differential Revision: https://reviews.llvm.org/D91778 --- libcxx/docs/Cxx2aStatus.rst | 1 + libcxx/docs/Cxx2aStatusPaperStatus.csv | 2 +- libcxx/include/__config | 6 + libcxx/include/string | 132 ++++++++++++--------- .../basic.string/string.capacity/reserve.pass.cpp | 50 ++++++++ .../reserve.deprecated_in_cxx20.verify.cpp | 22 ++++ .../basic.string/string.capacity/reserve.pass.cpp | 104 +++------------- .../string.capacity/reserve_size.pass.cpp | 110 +++++++++++++++++ 8 files changed, 284 insertions(+), 143 deletions(-) create mode 100644 libcxx/test/libcxx/strings/basic.string/string.capacity/reserve.pass.cpp create mode 100644 libcxx/test/std/strings/basic.string/string.capacity/reserve.deprecated_in_cxx20.verify.cpp create mode 100644 libcxx/test/std/strings/basic.string/string.capacity/reserve_size.pass.cpp diff --git a/libcxx/docs/Cxx2aStatus.rst b/libcxx/docs/Cxx2aStatus.rst index 562250ceb..4fd4e35 100644 --- a/libcxx/docs/Cxx2aStatus.rst +++ b/libcxx/docs/Cxx2aStatus.rst @@ -40,6 +40,7 @@ Paper Status .. [#note-P0202] P0202: The missing bits in P0202 are in ``copy`` and ``copy_backwards`` (and the ones that call them: ``copy_n``, ``set_union``, ``set_difference``, and ``set_symmetric_difference``). This is because the first two algorithms have specializations that call ``memmove`` which is not constexpr. See `Bug 25165 `__ .. [#note-P0600] P0600: The missing bits in P0600 are in |sect|\ [mem.res.class], |sect|\ [mem.poly.allocator.class], and |sect|\ [container.node.overview]. + .. [#note-P0966] P0966: It was previously erroneously marked as complete in version 8.0. See `bug 45368 `__. .. [#note-P0619] P0619: Only ``std::allocator`` part is implemented. diff --git a/libcxx/docs/Cxx2aStatusPaperStatus.csv b/libcxx/docs/Cxx2aStatusPaperStatus.csv index ee7acab..cf476c8 100644 --- a/libcxx/docs/Cxx2aStatusPaperStatus.csv +++ b/libcxx/docs/Cxx2aStatusPaperStatus.csv @@ -24,7 +24,7 @@ "`P0809R0 `__","LWG","Comparing Unordered Containers","Jacksonville","","" "`P0858R0 `__","LWG","Constexpr iterator requirements","Jacksonville","","" "`P0905R1 `__","CWG","Symmetry for spaceship","Jacksonville","","" -"`P0966R1 `__","LWG","``string::reserve``\ Should Not Shrink","Jacksonville","|Complete|","8.0" +"`P0966R1 `__","LWG","``string::reserve``\ Should Not Shrink","Jacksonville","|Complete| [#note-P0966]_","12.0" "","","","","","" "`P0019R8 `__","LWG","Atomic Ref","Rapperswil","","" "`P0458R2 `__","LWG","Checking for Existence of an Element in Associative Containers","Rapperswil","|Complete|","" diff --git a/libcxx/include/__config b/libcxx/include/__config index 069fc41..de40ffc 100644 --- a/libcxx/include/__config +++ b/libcxx/include/__config @@ -991,6 +991,12 @@ typedef unsigned int char32_t; # define _LIBCPP_DEPRECATED_IN_CXX17 #endif +#if _LIBCPP_STD_VER > 17 +# define _LIBCPP_DEPRECATED_IN_CXX20 _LIBCPP_DEPRECATED +#else +# define _LIBCPP_DEPRECATED_IN_CXX20 +#endif + // Macros to enter and leave a state where deprecation warnings are suppressed. #if !defined(_LIBCPP_SUPPRESS_DEPRECATED_PUSH) && \ (defined(_LIBCPP_COMPILER_CLANG) || defined(_LIBCPP_COMPILER_GCC)) diff --git a/libcxx/include/string b/libcxx/include/string index 9f7a2a9..d3e5359 100644 --- a/libcxx/include/string +++ b/libcxx/include/string @@ -153,7 +153,8 @@ public: void resize(size_type n, value_type c); void resize(size_type n); - void reserve(size_type res_arg = 0); + void reserve(size_type res_arg); + void reserve(); // deprecated in C++20 void shrink_to_fit(); void clear() noexcept; bool empty() const noexcept; @@ -954,13 +955,13 @@ public: void resize(size_type __n, value_type __c); _LIBCPP_INLINE_VISIBILITY void resize(size_type __n) {resize(__n, value_type());} - void reserve(size_type __res_arg); + void reserve(size_type __requested_capacity); _LIBCPP_INLINE_VISIBILITY void __resize_default_init(size_type __n); + _LIBCPP_DEPRECATED_IN_CXX20 _LIBCPP_INLINE_VISIBILITY + void reserve() _NOEXCEPT {shrink_to_fit();} _LIBCPP_INLINE_VISIBILITY - void reserve() _NOEXCEPT {reserve(0);} - _LIBCPP_INLINE_VISIBILITY - void shrink_to_fit() _NOEXCEPT {reserve();} + void shrink_to_fit() _NOEXCEPT; _LIBCPP_INLINE_VISIBILITY void clear() _NOEXCEPT; _LIBCPP_NODISCARD_AFTER_CXX17 _LIBCPP_INLINE_VISIBILITY @@ -1418,6 +1419,8 @@ public: _LIBCPP_INLINE_VISIBILITY void __clear_and_shrink() _NOEXCEPT; + _LIBCPP_INLINE_VISIBILITY void __shrink_or_extend(size_type __target_capacity); + _LIBCPP_INLINE_VISIBILITY bool __is_long() const _NOEXCEPT {return bool(__r_.first().__s.__size_ & __short_mask);} @@ -3262,65 +3265,88 @@ basic_string<_CharT, _Traits, _Allocator>::max_size() const _NOEXCEPT template void -basic_string<_CharT, _Traits, _Allocator>::reserve(size_type __res_arg) +basic_string<_CharT, _Traits, _Allocator>::reserve(size_type __requested_capacity) { - if (__res_arg > max_size()) + if (__requested_capacity > max_size()) this->__throw_length_error(); + +#if _LIBCPP_STD_VER > 17 + // Reserve never shrinks as of C++20. + if (__requested_capacity <= capacity()) return; +#endif + + size_type __target_capacity = _VSTD::max(__requested_capacity, size()); + __target_capacity = __recommend(__target_capacity); + if (__target_capacity == capacity()) return; + + __shrink_or_extend(__target_capacity); +} + +template +void +basic_string<_CharT, _Traits, _Allocator>::shrink_to_fit() _NOEXCEPT +{ + size_type __target_capacity = __recommend(size()); + if (__target_capacity == capacity()) return; + + __shrink_or_extend(__target_capacity); +} + +template +void +basic_string<_CharT, _Traits, _Allocator>::__shrink_or_extend(size_type __target_capacity) +{ size_type __cap = capacity(); size_type __sz = size(); - __res_arg = _VSTD::max(__res_arg, __sz); - __res_arg = __recommend(__res_arg); - if (__res_arg != __cap) + + pointer __new_data, __p; + bool __was_long, __now_long; + if (__target_capacity == __min_cap - 1) { - pointer __new_data, __p; - bool __was_long, __now_long; - if (__res_arg == __min_cap - 1) - { - __was_long = true; - __now_long = false; - __new_data = __get_short_pointer(); - __p = __get_long_pointer(); - } + __was_long = true; + __now_long = false; + __new_data = __get_short_pointer(); + __p = __get_long_pointer(); + } + else + { + if (__target_capacity > __cap) + __new_data = __alloc_traits::allocate(__alloc(), __target_capacity+1); else { - if (__res_arg > __cap) - __new_data = __alloc_traits::allocate(__alloc(), __res_arg+1); - else + #ifndef _LIBCPP_NO_EXCEPTIONS + try { - #ifndef _LIBCPP_NO_EXCEPTIONS - try - { - #endif // _LIBCPP_NO_EXCEPTIONS - __new_data = __alloc_traits::allocate(__alloc(), __res_arg+1); - #ifndef _LIBCPP_NO_EXCEPTIONS - } - catch (...) - { - return; - } - #else // _LIBCPP_NO_EXCEPTIONS - if (__new_data == nullptr) - return; - #endif // _LIBCPP_NO_EXCEPTIONS + #endif // _LIBCPP_NO_EXCEPTIONS + __new_data = __alloc_traits::allocate(__alloc(), __target_capacity+1); + #ifndef _LIBCPP_NO_EXCEPTIONS } - __now_long = true; - __was_long = __is_long(); - __p = __get_pointer(); - } - traits_type::copy(_VSTD::__to_address(__new_data), - _VSTD::__to_address(__p), size()+1); - if (__was_long) - __alloc_traits::deallocate(__alloc(), __p, __cap+1); - if (__now_long) - { - __set_long_cap(__res_arg+1); - __set_long_size(__sz); - __set_long_pointer(__new_data); + catch (...) + { + return; + } + #else // _LIBCPP_NO_EXCEPTIONS + if (__new_data == nullptr) + return; + #endif // _LIBCPP_NO_EXCEPTIONS } - else - __set_short_size(__sz); - __invalidate_all_iterators(); + __now_long = true; + __was_long = __is_long(); + __p = __get_pointer(); + } + traits_type::copy(_VSTD::__to_address(__new_data), + _VSTD::__to_address(__p), size()+1); + if (__was_long) + __alloc_traits::deallocate(__alloc(), __p, __cap+1); + if (__now_long) + { + __set_long_cap(__target_capacity+1); + __set_long_size(__sz); + __set_long_pointer(__new_data); } + else + __set_short_size(__sz); + __invalidate_all_iterators(); } template diff --git a/libcxx/test/libcxx/strings/basic.string/string.capacity/reserve.pass.cpp b/libcxx/test/libcxx/strings/basic.string/string.capacity/reserve.pass.cpp new file mode 100644 index 0000000..358f51f --- /dev/null +++ b/libcxx/test/libcxx/strings/basic.string/string.capacity/reserve.pass.cpp @@ -0,0 +1,50 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +// + +// void reserve(); // Deprecated in C++20. + +// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS + +#include +#include +#include + +#include "test_macros.h" +#include "min_allocator.h" + +template +void +test() +{ + // Tests that a call to reserve() on a long string is equivalent to shrink_to_fit(). + S s(1000, 'a'); + typename S::size_type old_cap = s.capacity(); + s.resize(20); + assert(s.capacity() == old_cap); + s.reserve(); + assert(s.capacity() < old_cap); +} + +int main(int, char**) +{ + { + typedef std::string S; + test(); + } +#if TEST_STD_VER >= 11 + { + typedef min_allocator A; + typedef std::basic_string, A> S; + test(); + } +#endif + + return 0; +} diff --git a/libcxx/test/std/strings/basic.string/string.capacity/reserve.deprecated_in_cxx20.verify.cpp b/libcxx/test/std/strings/basic.string/string.capacity/reserve.deprecated_in_cxx20.verify.cpp new file mode 100644 index 0000000..1b8a5da --- /dev/null +++ b/libcxx/test/std/strings/basic.string/string.capacity/reserve.deprecated_in_cxx20.verify.cpp @@ -0,0 +1,22 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +// + +// void reserve(); // Deprecated in C++20 + +// UNSUPPORTED: c++03, c++11, c++14, c++17 + +#include + +int main(int, char**) +{ + std::string s; + s.reserve(); // expected-warning {{'reserve' is deprecated}} + return 0; +} diff --git a/libcxx/test/std/strings/basic.string/string.capacity/reserve.pass.cpp b/libcxx/test/std/strings/basic.string/string.capacity/reserve.pass.cpp index f49125c..a200a63 100644 --- a/libcxx/test/std/strings/basic.string/string.capacity/reserve.pass.cpp +++ b/libcxx/test/std/strings/basic.string/string.capacity/reserve.pass.cpp @@ -8,9 +8,9 @@ // -// Split into two calls for C++20 -// void reserve(); -// void reserve(size_type res_arg); +// void reserve(); // Deprecated in C++20. + +// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS #include #include @@ -21,8 +21,13 @@ template void -test(S s) +test(typename S::size_type min_cap, typename S::size_type erased_index) { + S s(min_cap, 'a'); + s.erase(erased_index); + assert(s.size() == erased_index); + assert(s.capacity() >= min_cap); // Check that we really have at least this capacity. + typename S::size_type old_cap = s.capacity(); S s0 = s; s.reserve(); @@ -32,102 +37,23 @@ test(S s) assert(s.capacity() >= s.size()); } -template -void -test(S s, typename S::size_type res_arg) -{ - typename S::size_type old_cap = s.capacity(); - ((void)old_cap); // Prevent unused warning - S s0 = s; - if (res_arg <= s.max_size()) - { - s.reserve(res_arg); - assert(s == s0); - assert(s.capacity() >= res_arg); - assert(s.capacity() >= s.size()); -#if TEST_STD_VER > 17 - assert(s.capacity() >= old_cap); // resize never shrinks as of P0966 -#endif - } -#ifndef TEST_HAS_NO_EXCEPTIONS - else - { - try - { - s.reserve(res_arg); - assert(false); - } - catch (std::length_error&) - { - assert(res_arg > s.max_size()); - } - } -#endif -} - int main(int, char**) { { typedef std::string S; { - S s; - test(s); - - s.assign(10, 'a'); - s.erase(5); - test(s); - - s.assign(100, 'a'); - s.erase(50); - test(s); - } - { - S s; - test(s, 5); - test(s, 10); - test(s, 50); - } - { - S s(100, 'a'); - s.erase(50); - test(s, 5); - test(s, 10); - test(s, 50); - test(s, 100); - test(s, 1000); - test(s, S::npos); + test(0, 0); + test(10, 5); + test(100, 50); } } #if TEST_STD_VER >= 11 { typedef std::basic_string, min_allocator> S; { - S s; - test(s); - - s.assign(10, 'a'); - s.erase(5); - test(s); - - s.assign(100, 'a'); - s.erase(50); - test(s); - } - { - S s; - test(s, 5); - test(s, 10); - test(s, 50); - } - { - S s(100, 'a'); - s.erase(50); - test(s, 5); - test(s, 10); - test(s, 50); - test(s, 100); - test(s, 1000); - test(s, S::npos); + test(0, 0); + test(10, 5); + test(100, 50); } } #endif diff --git a/libcxx/test/std/strings/basic.string/string.capacity/reserve_size.pass.cpp b/libcxx/test/std/strings/basic.string/string.capacity/reserve_size.pass.cpp new file mode 100644 index 0000000..8820ad5a --- /dev/null +++ b/libcxx/test/std/strings/basic.string/string.capacity/reserve_size.pass.cpp @@ -0,0 +1,110 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +// + +// void reserve(size_type res_arg); + +// This test relies on https://llvm.org/PR45368 being fixed, which isn't in +// older Apple dylibs +// +// XFAIL: with_system_cxx_lib=macosx10.15 +// XFAIL: with_system_cxx_lib=macosx10.14 +// XFAIL: with_system_cxx_lib=macosx10.13 +// XFAIL: with_system_cxx_lib=macosx10.12 +// XFAIL: with_system_cxx_lib=macosx10.11 +// XFAIL: with_system_cxx_lib=macosx10.10 +// XFAIL: with_system_cxx_lib=macosx10.9 + +#include +#include +#include + +#include "test_macros.h" +#include "min_allocator.h" + +template +void +test(typename S::size_type min_cap, typename S::size_type erased_index, typename S::size_type res_arg) +{ + S s(min_cap, 'a'); + s.erase(erased_index); + assert(s.size() == erased_index); + assert(s.capacity() >= min_cap); // Check that we really have at least this capacity. + +#if TEST_STD_VER > 17 + typename S::size_type old_cap = s.capacity(); +#endif + S s0 = s; + if (res_arg <= s.max_size()) + { + s.reserve(res_arg); + LIBCPP_ASSERT(s.__invariants()); + assert(s == s0); + assert(s.capacity() >= res_arg); + assert(s.capacity() >= s.size()); +#if TEST_STD_VER > 17 + assert(s.capacity() >= old_cap); // reserve never shrinks as of P0966 (C++20) +#endif + } +#ifndef TEST_HAS_NO_EXCEPTIONS + else + { + try + { + s.reserve(res_arg); + LIBCPP_ASSERT(s.__invariants()); + assert(false); + } + catch (std::length_error&) + { + assert(res_arg > s.max_size()); + } + } +#endif +} + +int main(int, char**) +{ + { + typedef std::string S; + { + test(0, 0, 5); + test(0, 0, 10); + test(0, 0, 50); + } + { + test(100, 50, 5); + test(100, 50, 10); + test(100, 50, 50); + test(100, 50, 100); + test(100, 50, 1000); + test(100, 50, S::npos); + } + } +#if TEST_STD_VER >= 11 + { + typedef std::basic_string, min_allocator> S; + { + test(0, 0, 5); + test(0, 0, 10); + test(0, 0, 50); + } + { + test(100, 50, 5); + test(100, 50, 10); + test(100, 50, 50); + test(100, 50, 100); + test(100, 50, 1000); + test(100, 50, S::npos); + } + } +#endif + + return 0; +} -- 2.7.4