[libcxx-commits] [PATCH] D119112: [libc++] Implement P1165R1

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

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.



================
Comment at: libcxx/include/string:4199
 {
-    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()));
----------------
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.


================
Comment at: libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/allocator_propagation.pass.cpp:26-27
+
+  constexpr T* allocate(std::size_t n) { return std::allocator<value_type>().allocate(n); }
+  constexpr void deallocate(T* p, std::size_t s) { std::allocator<value_type>().deallocate(p, s); }
+
----------------
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.


================
Comment at: libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/allocator_propagation.pass.cpp:31
+    *soccc_count += 1;
+    return soccc_allocator(soccc_count);
+  }
----------------
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.


================
Comment at: libcxx/test/std/strings/basic.string/string.nonmembers/string_op+/allocator_propagation.pass.cpp:145
+    S rhs(soccc_allocator<char>{&soccc_rhs});
+    [[maybe_unused]] auto r = lhs[0] + std::move(rhs);
+    assert(soccc_lhs == 0);
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119112/new/

https://reviews.llvm.org/D119112