[libcxx-commits] [PATCH] D68365: Prototype implementation of P1004R2 (making std::vector constexpr).

[Michael Schellenberger Costa via Phabricator via libcxx-commits]

miscco added a comment.

Some quick review of the the acutal changes to the headers



================
Comment at: libcxx/include/memory:1390-1401
+template <class _Tp>
+_LIBCPP_CONSTEXPR_AFTER_CXX17
+_Tp* __libcpp_memcpy(_Tp* __dest, _Tp const* __src, std::size_t __count) {
+    if (__libcpp_is_constant_evaluated()) {
+        for (std::size_t __n = 0; __n != __count; ++__n)
+            __dest[__n] = __src[__n];
+    } else {
----------------
I am wondering whether we should reuse the simplementation of std::copy_n here (or at the call site for what its worth)


================
Comment at: libcxx/include/memory:1748
     if (_Np > 0) {
-        _VSTD::memcpy(__begin2, __begin1, _Np * sizeof(_Tp));
+        _VSTD::__libcpp_memcpy(__begin2, __begin1, _Np * sizeof(_Tp));
         __begin2 += _Np;
----------------
As said above, we should be able to reuse the internals of std::copy_n here


================
Comment at: libcxx/include/memory:1984
 
-
-  _LIBCPP_INLINE_VISIBILITY reference __get() _NOEXCEPT { return __value_; }
-  _LIBCPP_INLINE_VISIBILITY
+  _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX11
+  reference __get() _NOEXCEPT { return __value_; }
----------------
>From here and below  there is `_LIBCPP_CONSTEXPR_AFTER_CXX11` rather than `_LIBCPP_CONSTEXPR_AFTER_CXX17 `

Also the ordering of `_LIBCPP_INLINE_VISIBILITY ` and `_LIBCPP_CONSTEXPR_AFTER_CXX11` is swapped.


================
Comment at: libcxx/include/memory:2102
 template <class _T1, class _T2>
-inline _LIBCPP_INLINE_VISIBILITY
+inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX11
 void swap(__compressed_pair<_T1, _T2>& __x, __compressed_pair<_T1, _T2>& __y)
----------------
I believe the inline is superfluous with the `_LIBCPP_CONSTEXPR_AFTER_CXX17`

Appears throughout


================
Comment at: libcxx/include/vector:422
 template <class _Tp, class _Allocator>
-inline _LIBCPP_INLINE_VISIBILITY
+_LIBCPP_CONSTEXPR_AFTER_CXX17 inline _LIBCPP_INLINE_VISIBILITY
 void
----------------
Many inline below here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68365