From 6368f07dc23924f0bb8cc3b562eec960c6c6aea7 Mon Sep 17 00:00:00 2001 From: Michael Schellenberger Costa Date: Sat, 13 Mar 2021 09:09:44 +0100 Subject: [PATCH 1/5] Allow for SSO for constexpr string --- stl/inc/xstring | 87 ++++--------------- .../tests/P0980R1_constexpr_strings/test.cpp | 14 ++- 2 files changed, 20 insertions(+), 81 deletions(-) diff --git a/stl/inc/xstring b/stl/inc/xstring index ff946f56a52..1572415f747 100644 --- a/stl/inc/xstring +++ b/stl/inc/xstring @@ -2279,11 +2279,6 @@ public: } _CONSTEXPR20 bool _Large_string_engaged() const noexcept { -#if _HAS_CXX20 - if (_STD is_constant_evaluated()) { - return true; - } -#endif // _HAS_CXX20 return _BUF_SIZE <= _Myres; } @@ -2311,7 +2306,7 @@ public: } union _Bxty { // storage for small buffer or pointer to larger one - _CONSTEXPR20 _Bxty() noexcept : _Ptr() {} // user-provided, for fancy pointers + _CONSTEXPR20 _Bxty() noexcept : _Buf() {} // user-provided, for fancy pointers _CONSTEXPR20 ~_Bxty() noexcept {} // user-provided, for fancy pointers @@ -2689,19 +2684,8 @@ private: auto&& _Alproxy = _GET_PROXY_ALLOCATOR(_Alty, _Al); _Container_proxy_ptr<_Alty> _Proxy(_Alproxy, _My_data); + if (_Count < _BUF_SIZE) { #if _HAS_CXX20 - if (_STD is_constant_evaluated()) { - _My_data._Myres = _BUF_SIZE; // TRANSITION: constexpr SSO - } - - const bool _Stay_small = _Count < _BUF_SIZE && !_STD is_constant_evaluated(); -#else // ^^^ _HAS_CXX20 / !_HAS_CXX20 vvv - const bool _Stay_small = _Count < _BUF_SIZE; -#endif // _HAS_CXX20 - - if (_Stay_small) { -#if _HAS_CXX20 - // TRANSITION: This is currently unused until SSO support is merged if (_STD is_constant_evaluated()) { _Construct_in_place(_My_data._Bx); } @@ -2767,7 +2751,6 @@ private: #if _HAS_CXX20 if (_STD is_constant_evaluated()) { _Construct_in_place(_My_data._Bx); - _My_data._Myres = _BUF_SIZE; // TRANSITION: constexpr SSO } #endif // _HAS_CXX20 @@ -2777,12 +2760,7 @@ private: _Xlen_string(); // result too long } -#if _HAS_CXX20 - const bool _Become_large = _Count >= _BUF_SIZE || _STD is_constant_evaluated(); -#else // ^^^ _HAS_CXX20 / !_HAS_CXX20 vvv - const bool _Become_large = _Count >= _BUF_SIZE; -#endif // _HAS_CXX20 - if (_Become_large) { + if (_Count >= _BUF_SIZE) { const size_type _New_capacity = _Calculate_growth(_Count); const pointer _New_ptr = _Al.allocate(_New_capacity + 1); // throws _Construct_in_place(_My_data._Bx._Ptr, _New_ptr); @@ -2873,13 +2851,7 @@ public: auto&& _Alproxy = _GET_PROXY_ALLOCATOR(_Alty, _Getal()); _Container_proxy_ptr<_Alty> _Proxy(_Alproxy, _My_data); // throws -#if _HAS_CXX20 - const bool _Activate_large_mode = _New_capacity < _New_size || _STD is_constant_evaluated(); -#else // ^^^ _HAS_CXX20 / !_HAS_CXX20 vvv - const bool _Activate_large_mode = _New_capacity < _New_size; -#endif // _HAS_CXX20 - - if (_Activate_large_mode) { + if (_New_capacity < _New_size) { // we should never allocate less than _BUF_SIZE space (_New_size could be small if constant evaluated) const size_type _Requested_size = (_STD max)(_New_size, _BUF_SIZE); _New_capacity = _Calculate_growth(_Requested_size, _BUF_SIZE - 1, max_size()); @@ -3222,9 +3194,6 @@ public: private: _CONSTEXPR20 void _Copy_assign_val_from_small(const basic_string& _Right) { // TRANSITION, VSO-761321; inline into only caller when that's fixed -#if _HAS_CXX20 - _STL_ASSERT(!_STD is_constant_evaluated(), "SSO should be disabled in a constexpr context"); -#endif // _HAS_CXX20 _Tidy_deallocate(); if constexpr (_Can_memcpy_val) { #if _HAS_CXX20 @@ -4007,22 +3976,12 @@ public: return; } -#if _HAS_CXX20 - const bool _Do_become_small = _My_data._Mysize < _BUF_SIZE && !_STD is_constant_evaluated(); -#else // ^^^ _HAS_CXX20 / !_HAS_CXX20 vvv - const bool _Do_become_small = _My_data._Mysize < _BUF_SIZE; -#endif // _HAS_CXX20 - if (_Do_become_small) { + if (_My_data._Mysize < _BUF_SIZE) { _Become_small(); return; } - size_type _Target_capacity = (_STD min)(_My_data._Mysize | _ALLOC_MASK, max_size()); -#if _HAS_CXX20 - // must allocate at least _BUF_SIZE space - _Target_capacity = (_STD max)(_Target_capacity, _BUF_SIZE); -#endif // _HAS_CXX20 - + const size_type _Target_capacity = (_STD min)(_My_data._Mysize | _ALLOC_MASK, max_size()); if (_Target_capacity < _My_data._Myres) { // worth shrinking, do it auto& _Al = _Getal(); const pointer _New_ptr = _Al.allocate(_Target_capacity + 1); // throws @@ -4872,24 +4831,15 @@ private: _CONSTEXPR20 void _Tidy_init() noexcept { // initialize basic_string data members auto& _My_data = _Mypair._Myval2; _My_data._Mysize = 0; - + _My_data._Myres = _BUF_SIZE - 1; #if _HAS_CXX20 - if (_STD is_constant_evaluated()) { - _My_data._Myres = _BUF_SIZE; // SSO disabled in constexpr context - auto& _Al = _Getal(); - const pointer _New_ptr = _Al.allocate(_BUF_SIZE + 1); // throws - _My_data._Bx._Ptr = _New_ptr; - - _Elem* const _Raw_new = _Unfancy(_New_ptr); - _Traits::assign(_Raw_new, _BUF_SIZE + 1, _Elem()); - } else -#endif // _HAS_CXX20 - { - _My_data._Myres = _BUF_SIZE - 1; - // the _Traits::assign is last so the codegen doesn't think the char write can alias this - _Traits::assign(_My_data._Bx._Buf[0], _Elem()); + if (_STD is_constant_evaluated()) { // begin the lifetime of the array elements before copying into them + _Construct_in_place(_My_data._Bx); } +#endif // _HAS_CXX20 + // the _Traits::assign is last so the codegen doesn't think the char write can alias this + _Traits::assign(_My_data._Bx._Buf[0], _Elem()); _ASAN_STRING_CREATE(*this); } @@ -4910,16 +4860,9 @@ private: } _My_data._Mysize = 0; -#if _HAS_CXX20 - if (_STD is_constant_evaluated()) { - _My_data._Myres = 0; - } else -#endif // _HAS_CXX20 - { - _My_data._Myres = _BUF_SIZE - 1; - // the _Traits::assign is last so the codegen doesn't think the char write can alias this - _Traits::assign(_My_data._Bx._Buf[0], _Elem()); - } + _My_data._Myres = _BUF_SIZE - 1; + // the _Traits::assign is last so the codegen doesn't think the char write can alias this + _Traits::assign(_My_data._Bx._Buf[0], _Elem()); } public: diff --git a/tests/std/tests/P0980R1_constexpr_strings/test.cpp b/tests/std/tests/P0980R1_constexpr_strings/test.cpp index 6ccf0a5ce6b..b0cac4cdec4 100644 --- a/tests/std/tests/P0980R1_constexpr_strings/test.cpp +++ b/tests/std/tests/P0980R1_constexpr_strings/test.cpp @@ -613,16 +613,12 @@ constexpr bool test_interface() { literal_constructed.shrink_to_fit(); const auto c4 = literal_constructed.capacity(); - if (is_constant_evaluated()) { // check minimum allocation of _BUF_SIZE when constant evaluated - assert(c4 == 16 / sizeof(CharType)); + if constexpr (is_same_v || is_same_v) { + assert(c4 == 7); + } else if constexpr (is_same_v) { + assert(c4 == 3); } else { - if constexpr (is_same_v || is_same_v) { - assert(c4 == 7); - } else if constexpr (is_same_v) { - assert(c4 == 3); - } else { - assert(c4 == 15); - } + assert(c4 == 15); } } From e5a45a8371329fd5fc7c26490b7b627fb765d776 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 16 Jun 2022 17:39:33 -0700 Subject: [PATCH 2/5] Drop one more constexpr SSO workaround. --- stl/inc/xstring | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/stl/inc/xstring b/stl/inc/xstring index 1572415f747..7bff8349e2b 100644 --- a/stl/inc/xstring +++ b/stl/inc/xstring @@ -2852,11 +2852,9 @@ public: _Container_proxy_ptr<_Alty> _Proxy(_Alproxy, _My_data); // throws if (_New_capacity < _New_size) { - // we should never allocate less than _BUF_SIZE space (_New_size could be small if constant evaluated) - const size_type _Requested_size = (_STD max)(_New_size, _BUF_SIZE); - _New_capacity = _Calculate_growth(_Requested_size, _BUF_SIZE - 1, max_size()); - const pointer _Fancyptr = _Getal().allocate(_New_capacity + 1); // throws - _Ptr = _Unfancy(_Fancyptr); + _New_capacity = _Calculate_growth(_New_size, _BUF_SIZE - 1, max_size()); + const pointer _Fancyptr = _Getal().allocate(_New_capacity + 1); // throws + _Ptr = _Unfancy(_Fancyptr); _Construct_in_place(_My_data._Bx._Ptr, _Fancyptr); #if _HAS_CXX20 From 806301f98c4f2953936ec2b60e73ecf5157edfc7 Mon Sep 17 00:00:00 2001 From: Casey Carter Date: Fri, 17 Jun 2022 22:57:35 -0700 Subject: [PATCH 3/5] Centralize code to activate SSO elements ... and do so without ending the lifetime of the enclosing `basic_string` by reusing its storage. --- stl/inc/xstring | 56 ++++++++++++++++++------------------------------- 1 file changed, 20 insertions(+), 36 deletions(-) diff --git a/stl/inc/xstring b/stl/inc/xstring index 7bff8349e2b..99af36e9853 100644 --- a/stl/inc/xstring +++ b/stl/inc/xstring @@ -2282,6 +2282,17 @@ public: return _BUF_SIZE <= _Myres; } + constexpr void _Activate_SSO_buffer() { + // begin the lifetime of the array elements (e.g., before copying into them) +#if _HAS_CXX20 + if (_STD is_constant_evaluated()) { + for (size_type _Idx = 0; _Idx < _BUF_SIZE; ++_Idx) { + _Bx._Buf[_Idx] = value_type(); + } + } +#endif // _HAS_CXX20 + } + _CONSTEXPR20 void _Check_offset(const size_type _Off) const { // checks whether _Off is in the bounds of [0, size()] if (_Mysize < _Off) { @@ -2669,6 +2680,8 @@ private: enum class _Construct_strategy : uint8_t { _From_char, _From_ptr, _From_string }; template <_Construct_strategy _Strat, class _Char_or_ptr> _CONSTEXPR20 void _Construct(const _Char_or_ptr _Arg, _CRT_GUARDOVERFLOW const size_type _Count) { + // Pre: *this is in SSO mode; the lifetime of the SSO elements has already begun + if constexpr (_Strat == _Construct_strategy::_From_char) { _STL_INTERNAL_STATIC_ASSERT(is_same_v<_Char_or_ptr, _Elem>); } else { @@ -2685,12 +2698,6 @@ private: _Container_proxy_ptr<_Alty> _Proxy(_Alproxy, _My_data); if (_Count < _BUF_SIZE) { -#if _HAS_CXX20 - if (_STD is_constant_evaluated()) { - _Construct_in_place(_My_data._Bx); - } -#endif // _HAS_CXX20 - _My_data._Mysize = _Count; _My_data._Myres = _BUF_SIZE - 1; if constexpr (_Strat == _Construct_strategy::_From_char) { @@ -2741,6 +2748,8 @@ private: template _CONSTEXPR20 void _Construct_from_iter(_Iter _First, const _Iter _Last) { + // Pre: *this is in SSO mode; the lifetime of the SSO elements has already begun + auto& _My_data = _Mypair._Myval2; auto& _Al = _Getal(); auto&& _Alproxy = _GET_PROXY_ALLOCATOR(_Alty, _Al); @@ -2748,11 +2757,6 @@ private: _My_data._Mysize = 0; _My_data._Myres = _BUF_SIZE - 1; -#if _HAS_CXX20 - if (_STD is_constant_evaluated()) { - _Construct_in_place(_My_data._Bx); - } -#endif // _HAS_CXX20 if constexpr (_Is_fwd_iter_v<_Iter>) { const auto _Count = _Convert_size(static_cast(_STD distance(_First, _Last))); @@ -3116,11 +3120,7 @@ private: _Right_data._Bx._Ptr = nullptr; _Swap_proxy_and_iterators(_Right); } else { // copy small string buffer -#if _HAS_CXX20 - if (_STD is_constant_evaluated()) { // begin the lifetime of the array elements before copying into them - _Construct_in_place(_Mypair._Myval2._Bx); - } -#endif // _HAS_CXX20 + _Mypair._Myval2._Activate_SSO_buffer(); _Traits::copy(_My_data._Bx._Buf, _Right_data._Bx._Buf, _Right_data._Mysize + 1); _Right_data._Orphan_all(); } @@ -4237,11 +4237,7 @@ public: // exchange a string in large mode with one in small mode const pointer _Ptr = _Starts_large._Bx._Ptr; _Destroy_in_place(_Starts_large._Bx._Ptr); -#if _HAS_CXX20 - if (_STD is_constant_evaluated()) { - _Construct_in_place(_Starts_large._Bx); - } -#endif // _HAS_CXX20 + _Starts_large._Activate_SSO_buffer(); _Traits::copy(_Starts_large._Bx._Buf, _Starts_small._Bx._Buf, _BUF_SIZE); _Construct_in_place(_Starts_small._Bx._Ptr, _Ptr); } @@ -4810,11 +4806,7 @@ private: const pointer _Ptr = _My_data._Bx._Ptr; auto& _Al = _Getal(); _Destroy_in_place(_My_data._Bx._Ptr); -#if _HAS_CXX20 - if (_STD is_constant_evaluated()) { // begin the lifetime of the array elements before copying into them - _Construct_in_place(_Mypair._Myval2._Bx); - } -#endif // _HAS_CXX20 + _Mypair._Myval2._Activate_SSO_buffer(); _Traits::copy(_My_data._Bx._Buf, _Unfancy(_Ptr), _My_data._Mysize + 1); _Al.deallocate(_Ptr, _My_data._Myres + 1); _My_data._Myres = _BUF_SIZE - 1; @@ -4830,11 +4822,7 @@ private: auto& _My_data = _Mypair._Myval2; _My_data._Mysize = 0; _My_data._Myres = _BUF_SIZE - 1; -#if _HAS_CXX20 - if (_STD is_constant_evaluated()) { // begin the lifetime of the array elements before copying into them - _Construct_in_place(_My_data._Bx); - } -#endif // _HAS_CXX20 + _My_data._Activate_SSO_buffer(); // the _Traits::assign is last so the codegen doesn't think the char write can alias this _Traits::assign(_My_data._Bx._Buf[0], _Elem()); @@ -4849,11 +4837,7 @@ private: const pointer _Ptr = _My_data._Bx._Ptr; auto& _Al = _Getal(); _Destroy_in_place(_My_data._Bx._Ptr); -#if _HAS_CXX20 - if (_STD is_constant_evaluated()) { // begin the lifetime of the array elements before copying into them - _Construct_in_place(_My_data._Bx); - } -#endif // _HAS_CXX20 + _My_data._Activate_SSO_buffer(); _Al.deallocate(_Ptr, _My_data._Myres + 1); } From 66eeb12e67f124323e858c5a66436b35af80aea8 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Sat, 18 Jun 2022 01:35:10 -0700 Subject: [PATCH 4/5] Even tinier code review feedback. --- stl/inc/xstring | 4 ++-- tests/std/tests/P0980R1_constexpr_strings/test.cpp | 8 +------- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/stl/inc/xstring b/stl/inc/xstring index 99af36e9853..313869c77f9 100644 --- a/stl/inc/xstring +++ b/stl/inc/xstring @@ -3120,7 +3120,7 @@ private: _Right_data._Bx._Ptr = nullptr; _Swap_proxy_and_iterators(_Right); } else { // copy small string buffer - _Mypair._Myval2._Activate_SSO_buffer(); + _My_data._Activate_SSO_buffer(); _Traits::copy(_My_data._Bx._Buf, _Right_data._Bx._Buf, _Right_data._Mysize + 1); _Right_data._Orphan_all(); } @@ -4806,7 +4806,7 @@ private: const pointer _Ptr = _My_data._Bx._Ptr; auto& _Al = _Getal(); _Destroy_in_place(_My_data._Bx._Ptr); - _Mypair._Myval2._Activate_SSO_buffer(); + _My_data._Activate_SSO_buffer(); _Traits::copy(_My_data._Bx._Buf, _Unfancy(_Ptr), _My_data._Mysize + 1); _Al.deallocate(_Ptr, _My_data._Myres + 1); _My_data._Myres = _BUF_SIZE - 1; diff --git a/tests/std/tests/P0980R1_constexpr_strings/test.cpp b/tests/std/tests/P0980R1_constexpr_strings/test.cpp index b0cac4cdec4..9dd46fb8222 100644 --- a/tests/std/tests/P0980R1_constexpr_strings/test.cpp +++ b/tests/std/tests/P0980R1_constexpr_strings/test.cpp @@ -613,13 +613,7 @@ constexpr bool test_interface() { literal_constructed.shrink_to_fit(); const auto c4 = literal_constructed.capacity(); - if constexpr (is_same_v || is_same_v) { - assert(c4 == 7); - } else if constexpr (is_same_v) { - assert(c4 == 3); - } else { - assert(c4 == 15); - } + assert(c4 == 16 / sizeof(CharType) - 1); } { // clear From 3d2e9cd68e6bc0e09a7866d91f43f5cdd0ba5ab6 Mon Sep 17 00:00:00 2001 From: Casey Carter Date: Sat, 18 Jun 2022 02:16:07 -0700 Subject: [PATCH 5/5] _Activate_SSO_buffer can/does not throw Co-authored-by: Michael Schellenberger Costa --- stl/inc/xstring | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stl/inc/xstring b/stl/inc/xstring index 313869c77f9..048541f3d08 100644 --- a/stl/inc/xstring +++ b/stl/inc/xstring @@ -2282,7 +2282,7 @@ public: return _BUF_SIZE <= _Myres; } - constexpr void _Activate_SSO_buffer() { + constexpr void _Activate_SSO_buffer() noexcept { // begin the lifetime of the array elements (e.g., before copying into them) #if _HAS_CXX20 if (_STD is_constant_evaluated()) {