[libcxx-commits] [PATCH] D58879: Implement uses-allocator (P0591R4)

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

Quuxplusone added inline comments.


================
Comment at: include/memory:5751
+    return _VSTD::make_tuple(_VSTD::piecewise_construct,
+                             _VSTD::apply([&__alloc](auto&&... __args_a) -> auto {
+                                return uses_allocator_construction_args<_First>(__alloc,
----------------
What is `-> auto` doing here and on line 5755?


================
Comment at: include/memory:5798
+                                                 piecewise_construct,
+                                                 _VSTD::forward_as_tuple(_VSTD::forward<_T1>(__p.first)),
+                                                 _VSTD::forward_as_tuple(_VSTD::forward<_T2>(__p.second)));
----------------
So if I pass in a `pair<string, int>` or `pair<string&&, int>`, you'll //move// out of `p.first`, but if I pass in `pair<string&, int>`, you'll make a //copy// of `p.first`, is that right? I guess that seems reasonable, but it's a surprising/smelly use of `std::forward` IMVHO.


================
Comment at: include/scoped_allocator:515
+#endif // _LIBCPP_STD_VER > 17
+        }
 
----------------
This function body is indented an extra level. Not your patch's fault, but I wonder if your patch should fix it.


================
Comment at: test/std/utilities/allocator.adaptor/allocator.adaptor.members/construct_pair.pass.cpp:49
+                                 std::tuple<std::allocator_arg_t const&, SA const&>&&,
+                                 std::tuple<SA const&>&&
               >(CA, ptr)));
----------------
What's the rationale for all these test changes? I strongly suspect that they are a bad idea: if the original tests fail after your patch, I think there must be something wrong with your patch.


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

https://reviews.llvm.org/D58879