[libcxx-commits] [PATCH] D61761: P1144 "Trivially relocatable" (1/3): is_trivially_relocatable, relocate_at, and uninitialized_relocate

Zoe Carver via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Sun May 12 20:11:07 PDT 2019


zoecarver added inline comments.


================
Comment at: include/memory:3464
+_Tp *__relocate_at_impl(_Tp *__source, _Tp *__dest, false_type) {
+    ::new (static_cast<void*>(__dest)) _Tp(_VSTD::move(*__dest));
+    __source->~_Tp();
----------------
Quuxplusone wrote:
> zoecarver wrote:
> > Are you intentionally not doing anything with the result of `new`? Also, I am not sure you need to `static_cast` to `void*`.
> > 
> > Also, also, I think you want to move `__source` not `__dest` (because I am pretty sure `__dest` is where you want to relocate the new thing and `__source` is where you want to get it).
> Yikes! That was a pretty bad bug. Thanks.
Of course! That's what code review is for :) 

Maybe add a test for this though. 


================
Comment at: include/memory:3518
+_ForwardIt2
+__uninitialized_relocate_impl(_ForwardIt1 __first, _ForwardIt1 __last,
+                              _ForwardIt2 __result,
----------------
Quuxplusone wrote:
> zoecarver wrote:
> > I don't think this overload is necessary in addition to the below one.
> I think this overload would never really be useful in practice, but since it does something different from the below one (element-wise memcpy in a loop, instead of one big memcpy), I figured I should implement it.
> 
> However, since P1144R3 adds `relocate_at`, I can refactor this overload and the one //above// into a single overload that does `relocate_at`-in-a-loop! Will do.
> 
> In the process of rewriting this code, I realized that whereas `uninitialized_relocate` copies `uninitialized_move` in not requiring the two iterator types to have the same `value_type`, `relocate_at` requires the exact same type `_Tp *` for both `__source` and `__dest`. I have added an internal version `__relocate_at2(_Sp *__source, _Dp *__dest)`, and am ambivalently considering adding it to the proposal P1144R4. That would require some kind of change to the word "homogeneous" in P1144R3, though. Feel free to comment on this idea here and/or by email.
IMVHO I think it would be better to have them be one type and let the user `static_cast` the pointer if they want something different. 

If the difference is meaningful, maybe another overload should be added in P1144R3. Otherwise, I think combining them and using `relocate_at` is a good idea.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D61761





More information about the libcxx-commits mailing list