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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked 4 inline comments as done.
zoecarver 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,
----------------
Quuxplusone wrote:
> What is `-> auto` doing here and on line 5755?
Will fix. 


================
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)));
----------------
Quuxplusone wrote:
> 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.
You are right; I should probably use `std::move` here. For reference [[ http://eel.is/c++draft/allocator.uses.construction#15 | here is what the standard says ]]. 


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


================
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)));
----------------
Quuxplusone wrote:
> 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.
These are all simply adding `const` or changing the reference type (I am not saying that necessarily makes them okay though). Because of how `uses_allocator_construction_args` is defined in the standard, `make_tuple` is passed an rvalue `piecewise_construct` (that is why the first part of this changed). I think the others are similar. 

I will take a closer look at this and see which test is correct. 


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

https://reviews.llvm.org/D58879