[libc++] Implement P1165R1 (Make stateful allocator propagation more consistent)

[philnik]

• Don't use macros for constexpr

[• Quuxplusone]

It would help to mention the paper's title "Make stateful allocator propagation more consistent", in the PR's topic line.

LGTM modulo test nits. The table in p1165r1 says libc++ needs to change in exactly 5 places, and I see exactly 5 places being changed here, so that seems good to me.

libcxx/include/string

4216–4218

Throughout, I'd prefer to see _String (or even _Self) instead of __string, to avoid confusion with std::string and <__string>. But maybe that's controversial, I'm not sure.

libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/allocator_propagation.pass.cpp

27–28

Nit: Since the first mention on the line is of T, I'd prefer if s/value_type/T/ on the second mention as well.

32

It would be slightly better if you could find a way to distinguish

returned_alloc = lhs_alloc.select_on_container_copy_construction();

from

(void) lhs_alloc.select_on_container_copy_construction();
returned_alloc = lhs_alloc;

i.e., your select_on_container_copy_construction should do something observably different from just making a straight copy. I'm not sure what that would be, though. Maybe here return soccc_allocator(soccc_count + 1)... but that seems too "cute/clever" to me. You might have to add a second data member for that purpose. It might not be worth the bother.

146

In tests, [[maybe_unused]] should be considered a code smell. Don't you need to assert(r.get_allocator().soccc_count == &soccc_rhs) afterward, i.e. the correct allocator got propagated? If you check every postcondition, [[maybe_unused]] should basically never be necessary.

[philnik]

• Address comments

[• Quuxplusone]

The get_soccc() getter may be overkill (versus just making those members public). Consider making the getters const-qualified. But LGTM!

[ldionne]

We need to update the status page, and also please add a release note!

libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/allocator_propagation.pass.cpp

11

Add:

// This test ensures that we properly propagate allocators, per https://wg21.link/p1165r1

45

Can we template that on CharT and run the test for both char and wchar_t (if !defined(TEST_HAS_NO_WIDE_CHARACTERS))? The reason I'm asking is that we explicitly instantiate some string methods in the dylib for char, but not for wchar_t. We expect the behavior for char and wchar_t to be the same, however I could imagine a case where they don't match (for example in back-deployment cases for char, since operator+ is in the dylib). Adding wchar_t will help shake out these potential differences.

Incidentally, I don't understand why this test passes on Apple backdeployment, since I would expect it to use operator+ in the dylib, which doesn't do SOCCC properly.

[ldionne]

libcxx/include/string

4164–4167

Is there any reason why we don't implement those naively like in the Standard? I don't think there's any performance gain here, right? This applies to all the overloads AFAICT.

[philnik]

• Address comments

[ldionne]

libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/allocator_propagation.pass.cpp

191

wchar_t

[philnik]

• Address comment

libcxx/include/string

4164–4167

IIUC this implementation makes only one allocation, while making a copy and appending makes two allocations.

[philnik]

• Fix CI

[ldionne]

libcxx/include/string

4164–4167

Oh, right, because we already reserve enough space for the append to be allocation-free. Makes sense.

libcxx/docs/ReleaseNotes.rst

 
 What's New in Libc++ 15.0.0?
 ============================
 
 New Features
 ------------
 
  - Implemented P0627R6 (Function to mark unreachable code)
+ - Implemented P1165R1 (Make stateful allocator propagation more consistent for operator+(basic_string))
 
 API Changes
 -----------
 
 - The ``_LIBCPP_ABI_UNSTABLE`` macro has been removed in favour of setting
   ``_LIBCPP_ABI_VERSION=2``. This should not have any impact on users because
   they were not supposed to set ``_LIBCPP_ABI_UNSTABLE`` manually, however we
   still feel that it is worth mentioning in the release notes in case some users

libcxx/docs/Status/Cxx20Papers.csv

 "`P0972R0 <https://wg21.link/P0972R0>`__","LWG","<chrono> ``zero()``\ , ``min()``\ , and ``max()``\  should be noexcept","San Diego","|Complete|","8.0"
 "`P1006R1 <https://wg21.link/P1006R1>`__","LWG","Constexpr in std::pointer_traits","San Diego","|Complete|","8.0"
 "`P1007R3 <https://wg21.link/P1007R3>`__","LWG","``std::assume_aligned``\ ","San Diego","* *",""
 "`P1020R1 <https://wg21.link/P1020R1>`__","LWG","Smart pointer creation with default initialization","San Diego","* *",""
 "`P1032R1 <https://wg21.link/P1032R1>`__","LWG","Misc constexpr bits","San Diego","|Complete|","13.0"
 "`P1085R2 <https://wg21.link/P1085R2>`__","LWG","Should Span be Regular?","San Diego","|Complete|","8.0"
 "`P1123R0 <https://wg21.link/P1123R0>`__","LWG","Editorial Guidance for merging P0019r8 and P0528r3","San Diego","* *",""
 "`P1148R0 <https://wg21.link/P1148R0>`__","LWG","Cleaning up Clause 20","San Diego","* *",""
-"`P1165R1 <https://wg21.link/P1165R1>`__","LWG","Make stateful allocator propagation more consistent for ``operator+(basic_string)``\ ","San Diego","* *",""
+"`P1165R1 <https://wg21.link/P1165R1>`__","LWG","Make stateful allocator propagation more consistent for ``operator+(basic_string)``\ ","San Diego","|Complete|","15.0"
 "`P1209R0 <https://wg21.link/P1209R0>`__","LWG","Adopt Consistent Container Erasure from Library Fundamentals 2 for C++20","San Diego","|Complete|","8.0"
 "`P1210R0 <https://wg21.link/P1210R0>`__","LWG","Completing the Rebase of Library Fundamentals, Version 3, Working Draft","San Diego","* *",""
 "`P1236R1 <https://wg21.link/P1236R1>`__","CWG","Alternative Wording for P0907R4 Signed Integers are Two's Complement","San Diego","* *",""
 "`P1248R1 <https://wg21.link/P1248R1>`__","LWG","Remove CommonReference requirement from StrictWeakOrdering (a.k.a Fixing Relations)","San Diego","|Complete|","13.0"
 "`P1285R0 <https://wg21.link/P1285R0>`__","LWG","Improving Completeness Requirements for Type Traits","San Diego","* *",""
 "`P1353R0 <https://wg21.link/P1353R0>`__","CWG","Missing feature test macros","San Diego","* *",""
 "","","","","",""
 "`P0339R6 <https://wg21.link/P0339R6>`__","LWG","polymorphic_allocator<> as a vocabulary type","Kona","",""

libcxx/include/string

 
 // operator +
 
 template<class _CharT, class _Traits, class _Allocator>
 basic_string<_CharT, _Traits, _Allocator>
 operator+(const basic_string<_CharT, _Traits, _Allocator>& __lhs,
           const basic_string<_CharT, _Traits, _Allocator>& __rhs)
 {
-    basic_string<_CharT, _Traits, _Allocator> __r(__lhs.get_allocator());
-    typename basic_string<_CharT, _Traits, _Allocator>::size_type __lhs_sz = __lhs.size();
-    typename basic_string<_CharT, _Traits, _Allocator>::size_type __rhs_sz = __rhs.size();
+    using _String = basic_string<_CharT, _Traits, _Allocator>;
+    _String __r(_String::__alloc_traits::select_on_container_copy_construction(__lhs.get_allocator()));
+    typename _String::size_type __lhs_sz = __lhs.size();
+    typename _String::size_type __rhs_sz = __rhs.size();
     __r.__init(__lhs.data(), __lhs_sz, __lhs_sz + __rhs_sz);
     __r.append(__rhs.data(), __rhs_sz);
     return __r;
 }
 
 template<class _CharT, class _Traits, class _Allocator>
 basic_string<_CharT, _Traits, _Allocator>
 operator+(const _CharT* __lhs , const basic_string<_CharT,_Traits,_Allocator>& __rhs)
 {
-    basic_string<_CharT, _Traits, _Allocator> __r(__rhs.get_allocator());
-    typename basic_string<_CharT, _Traits, _Allocator>::size_type __lhs_sz = _Traits::length(__lhs);
-    typename basic_string<_CharT, _Traits, _Allocator>::size_type __rhs_sz = __rhs.size();
+    using _String = basic_string<_CharT, _Traits, _Allocator>;
+    _String __r(_String::__alloc_traits::select_on_container_copy_construction(__rhs.get_allocator()));
+    typename _String::size_type __lhs_sz = _Traits::length(__lhs);
+    typename _String::size_type __rhs_sz = __rhs.size();
     __r.__init(__lhs, __lhs_sz, __lhs_sz + __rhs_sz);
     __r.append(__rhs.data(), __rhs_sz);
     return __r;
 }
 
 template<class _CharT, class _Traits, class _Allocator>
 basic_string<_CharT, _Traits, _Allocator>
 operator+(_CharT __lhs, const basic_string<_CharT,_Traits,_Allocator>& __rhs)
 {
-    basic_string<_CharT, _Traits, _Allocator> __r(__rhs.get_allocator());
-    typename basic_string<_CharT, _Traits, _Allocator>::size_type __rhs_sz = __rhs.size();
+    using _String = basic_string<_CharT, _Traits, _Allocator>;
+    _String __r(_String::__alloc_traits::select_on_container_copy_construction(__rhs.get_allocator()));
+    typename _String::size_type __rhs_sz = __rhs.size();
     __r.__init(&__lhs, 1, 1 + __rhs_sz);
     __r.append(__rhs.data(), __rhs_sz);
     return __r;
 }
 
 template<class _CharT, class _Traits, class _Allocator>
 inline
 basic_string<_CharT, _Traits, _Allocator>
 operator+(const basic_string<_CharT, _Traits, _Allocator>& __lhs, const _CharT* __rhs)
 {
-    basic_string<_CharT, _Traits, _Allocator> __r(__lhs.get_allocator());
-    typename basic_string<_CharT, _Traits, _Allocator>::size_type __lhs_sz = __lhs.size();
-    typename basic_string<_CharT, _Traits, _Allocator>::size_type __rhs_sz = _Traits::length(__rhs);
+    using _String = basic_string<_CharT, _Traits, _Allocator>;
+    _String __r(_String::__alloc_traits::select_on_container_copy_construction(__lhs.get_allocator()));
+    typename _String::size_type __lhs_sz = __lhs.size();
+    typename _String::size_type __rhs_sz = _Traits::length(__rhs);
     __r.__init(__lhs.data(), __lhs_sz, __lhs_sz + __rhs_sz);
     __r.append(__rhs, __rhs_sz);
     return __r;
 }
 
 template<class _CharT, class _Traits, class _Allocator>
 basic_string<_CharT, _Traits, _Allocator>
 operator+(const basic_string<_CharT, _Traits, _Allocator>& __lhs, _CharT __rhs)
 {
-    basic_string<_CharT, _Traits, _Allocator> __r(__lhs.get_allocator());
-    typename basic_string<_CharT, _Traits, _Allocator>::size_type __lhs_sz = __lhs.size();
+    using _String = basic_string<_CharT, _Traits, _Allocator>;
+    _String __r(_String::__alloc_traits::select_on_container_copy_construction(__lhs.get_allocator()));
+    typename _String::size_type __lhs_sz = __lhs.size();
     __r.__init(__lhs.data(), __lhs_sz, __lhs_sz + 1);
     __r.push_back(__rhs);
     return __r;
 }
 
 #ifndef _LIBCPP_CXX03_LANG
 
 template<class _CharT, class _Traits, class _Allocator>

libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/allocator_propagation.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+
+// This test ensures that we properly propagate allocators, per https://wg21.link/p1165r1
+
+#include <cassert>
+#include <string>
+
+#include "test_macros.h"
+
+template <class T>
+class soccc_allocator {
+  int* soccc_count;
+  int self_soccc_count;
+
+public:
+  using value_type = T;
+
+  constexpr explicit soccc_allocator(int* soccc_count_, int self_coccc_count_ = 0)
+      : soccc_count(soccc_count_), self_soccc_count(self_coccc_count_) {}
+
+  template <class U>
+  constexpr soccc_allocator(const soccc_allocator<U>& a) : soccc_count(a.soccc_count) {}
+
+  constexpr T* allocate(std::size_t n) { return std::allocator<T>().allocate(n); }
+  constexpr void deallocate(T* p, std::size_t s) { std::allocator<T>().deallocate(p, s); }
+
+  constexpr soccc_allocator select_on_container_copy_construction() const {
+    *soccc_count += 1;
+    return soccc_allocator(soccc_count, self_soccc_count + 1);
+  }
+
+  constexpr auto get_soccc() { return soccc_count; }
+  constexpr auto get_self_soccc() { return self_soccc_count; }
+
+  typedef std::true_type propagate_on_container_copy_assignment;
+  typedef std::true_type propagate_on_container_move_assignment;
+  typedef std::true_type propagate_on_container_swap;
+};
+
+template <class CharT>
+bool test() {
+  using S = std::basic_string<CharT, std::char_traits<CharT>, soccc_allocator<CharT>>;
+  {
+    int soccc_lhs = 0;
+    int soccc_rhs = 0;
+    S lhs(soccc_allocator<CharT>{&soccc_lhs});
+    S rhs(soccc_allocator<CharT>{&soccc_rhs});
+    auto r = lhs + rhs;
+    assert(r.get_allocator().get_soccc() == &soccc_lhs);
+    assert(r.get_allocator().get_self_soccc() == 1);
+    assert(soccc_lhs == 1);
+    assert(soccc_rhs == 0);
+  }
+  {
+    int soccc_lhs = 0;
+    int soccc_rhs = 0;
+    S lhs(soccc_allocator<CharT>{&soccc_lhs});
+    S rhs(soccc_allocator<CharT>{&soccc_rhs});
+    auto r = lhs + std::move(rhs);
+    assert(r.get_allocator().get_soccc() == &soccc_rhs);
+    assert(r.get_allocator().get_self_soccc() == 0);
+    assert(soccc_lhs == 0);
+    assert(soccc_rhs == 0);
+  }
+  {
+    int soccc_lhs = 0;
+    int soccc_rhs = 0;
+    S lhs(soccc_allocator<CharT>{&soccc_lhs});
+    S rhs(soccc_allocator<CharT>{&soccc_rhs});
+    auto r = std::move(lhs) + rhs;
+    assert(r.get_allocator().get_soccc() == &soccc_lhs);
+    assert(r.get_allocator().get_self_soccc() == 0);
+    assert(soccc_lhs == 0);
+    assert(soccc_rhs == 0);
+  }
+  {
+    int soccc_lhs = 0;
+    int soccc_rhs = 0;
+    S lhs(soccc_allocator<CharT>{&soccc_lhs});
+    S rhs(soccc_allocator<CharT>{&soccc_rhs});
+    auto r = std::move(lhs) + std::move(rhs);
+    assert(r.get_allocator().get_soccc() == &soccc_lhs);
+    assert(r.get_allocator().get_self_soccc() == 0);
+    assert(soccc_lhs == 0);
+    assert(soccc_rhs == 0);
+  }
+  {
+    int soccc_lhs = 0;
+    int soccc_rhs = 0;
+    S lhs(soccc_allocator<CharT>{&soccc_lhs});
+    S rhs(soccc_allocator<CharT>{&soccc_rhs});
+    auto r = lhs + rhs.data();
+    assert(r.get_allocator().get_soccc() == &soccc_lhs);
+    assert(r.get_allocator().get_self_soccc() == 1);
+    assert(soccc_lhs == 1);
+    assert(soccc_rhs == 0);
+  }
+  {
+    int soccc_lhs = 0;
+    int soccc_rhs = 0;
+    S lhs(soccc_allocator<CharT>{&soccc_lhs});
+    S rhs(soccc_allocator<CharT>{&soccc_rhs});
+    auto r = lhs + rhs[0];
+    assert(r.get_allocator().get_soccc() == &soccc_lhs);
+    assert(r.get_allocator().get_self_soccc() == 1);
+    assert(soccc_lhs == 1);
+    assert(soccc_rhs == 0);
+  }
+  {
+    int soccc_lhs = 0;
+    int soccc_rhs = 0;
+    S lhs(soccc_allocator<CharT>{&soccc_lhs});
+    S rhs(soccc_allocator<CharT>{&soccc_rhs});
+    auto r = std::move(lhs) + rhs.data();
+    assert(r.get_allocator().get_soccc() == &soccc_lhs);
+    assert(r.get_allocator().get_self_soccc() == 0);
+    assert(soccc_lhs == 0);
+    assert(soccc_rhs == 0);
+  }
+  {
+    int soccc_lhs = 0;
+    int soccc_rhs = 0;
+    S lhs(soccc_allocator<CharT>{&soccc_lhs});
+    S rhs(soccc_allocator<CharT>{&soccc_rhs});
+    auto r = std::move(lhs) + rhs[0];
+    assert(r.get_allocator().get_soccc() == &soccc_lhs);
+    assert(r.get_allocator().get_self_soccc() == 0);
+    assert(soccc_lhs == 0);
+    assert(soccc_rhs == 0);
+  }
+  {
+    int soccc_lhs = 0;
+    int soccc_rhs = 0;
+    S lhs(soccc_allocator<CharT>{&soccc_lhs});
+    S rhs(soccc_allocator<CharT>{&soccc_rhs});
+    auto r = lhs.data() + rhs;
+    assert(r.get_allocator().get_soccc() == &soccc_rhs);
+    assert(r.get_allocator().get_self_soccc() == 1);
+    assert(soccc_lhs == 0);
+    assert(soccc_rhs == 1);
+  }
+  {
+    int soccc_lhs = 0;
+    int soccc_rhs = 0;
+    S lhs(soccc_allocator<CharT>{&soccc_lhs});
+    S rhs(soccc_allocator<CharT>{&soccc_rhs});
+    auto r = lhs[0] + rhs;
+    assert(r.get_allocator().get_soccc() == &soccc_rhs);
+    assert(r.get_allocator().get_self_soccc() == 1);
+    assert(soccc_lhs == 0);
+    assert(soccc_rhs == 1);
+  }
+  {
+    int soccc_lhs = 0;
+    int soccc_rhs = 0;
+    S lhs(soccc_allocator<CharT>{&soccc_lhs});
+    S rhs(soccc_allocator<CharT>{&soccc_rhs});
+    auto r = lhs.data() + std::move(rhs);
+    assert(r.get_allocator().get_soccc() == &soccc_rhs);
+    assert(r.get_allocator().get_self_soccc() == 0);
+    assert(soccc_lhs == 0);
+    assert(soccc_rhs == 0);
+  }
+  {
+    int soccc_lhs = 0;
+    int soccc_rhs = 0;
+    S lhs(soccc_allocator<CharT>{&soccc_lhs});
+    S rhs(soccc_allocator<CharT>{&soccc_rhs});
+    auto r = lhs[0] + std::move(rhs);
+    assert(r.get_allocator().get_soccc() == &soccc_rhs);
+    assert(r.get_allocator().get_self_soccc() == 0);
+    assert(soccc_lhs == 0);
+    assert(soccc_rhs == 0);
+  }
+
+  return true;
+}
+
+int main(int, char**) {
+  test<char>();
+#ifndef TEST_HAS_NO_WIDE_CHARACTERS
+  test<wchar_t>();
+#endif
+#if TEST_STD_VER > 17
+  // static_assert(test<char>());
+#ifndef TEST_HAS_NO_WIDE_CHARACTERS
+  // static_assert(test<wchar_t>());
+#endif
+#endif
+
+  return 0;
+}