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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked an inline comment as done.
zoecarver 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)));
----------------
Quuxplusone wrote:
> 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.)
I am not sure that is necessarily true. Sometimes one works, and the other doesn't. I think it should be pointed out that the author of the paper used `forward` in their implementation (probably for that reason). 


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

https://reviews.llvm.org/D58879