[libcxx-commits] [PATCH] D154613: [libc++] Fix std::move algorithm with trivial move-only types

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__string/constexpr_c_functions.h:186-189
+          is_copy_assignable<_Tp>::value    ? 0 :
+          is_move_assignable<_Tp>::value    ? 1 :
+          is_copy_constructible<_Tp>::value ? 2 :
+          is_move_constructible<_Tp>::value ? 3 : -1>());
----------------
Would it make sense to first try construction? Then we could use `__constexpr_memmove` for `uninitialized_{copy, move}` too.


================
Comment at: libcxx/include/__string/constexpr_c_functions.h:211
       for (; __count > 0; --__count)
-        __dest[__count - 1] = __src[__count - 1];
+        std::__assign_trivially_copyable(__dest + (__count - 1), __builtin_bit_cast(_Tp, __src[__count - 1]));
     } else {
----------------
Why do you `bit_cast` here? Also, could we add a `__bit_cast` instead of using builtins randomly in the code?


================
Comment at: libcxx/test/std/containers/sequences/vector/vector.modifiers/erase_iter.pass.cpp:40
     {
-    int a1[] = {1, 2, 3, 4, 5};
-    std::vector<int> l1(a1, a1+5);
-    l1.erase(l1.begin());
-    assert(is_contiguous_container_asan_correct(l1));
-    assert(l1 == std::vector<int>(a1+1, a1+5));
+        int a1[] = {1, 2, 3, 4, 5};
+        std::vector<int> l1(a1, a1+5);
----------------
Instead of moving all the code an indentation deeper, maybe move the braces an indentation higher? That would basically change this test to use two-space indentation instead of four spaces, like we do in new code. (or just clang-format the whole thing)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154613