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

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

Quuxplusone added inline comments.


================
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)));
----------------
zoecarver wrote:
> 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 ]]. 
Thanks for pointing to the relevant Standard wording! I was too lazy to go look for it myself. :)  The wording uses `std::move(pr).first`, which is equivalent to `std::forward<_T1>(pr.first)` but doesn't set off my red-flag detectors — I would greatly prefer `_VSTD::move(__p).first` here.

(Notice that `_VSTD::move(__p).first` is different from `_VSTD::move(__p.first)` when `__p` is a pair of lvalue references. Which is good; it's what we want.)


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

https://reviews.llvm.org/D58879