[libcxx-commits] [PATCH] D68365: [libc++] Implement P1004R2 (constexpr std::vector)

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a subscriber: var-const.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__bit_reference:358-361
+    if (__libcpp_is_constant_evaluated())
+        std::fill_n(std::__to_address(__first.__seg_), 0, __nw);
+    else
+        _VSTD::memset(_VSTD::__to_address(__first.__seg_), 0, __nw * sizeof(__storage_type));
----------------
I think we should always call `std::fill_n` here instead. Indeed, both are equivalent, but `std::fill_n` is nicer for various reasons.

I know `fill_n` doesn't end up calling `memset` directly, however the compiler will end up calling `memset` (when optimized). IIRC, @var-const was actually about to add this optimization to `fill_n` explicitly, when we realized it didn't have an impact on the code generation. @var-const can you confirm?

Also, I think we need to add a comment in `std::fill_n` that explains this. I think it can be done in this patch since we're switching from `memset` to `fill_n`.

This comment applies elsewhere in this patch too.


================
Comment at: libcxx/include/__bit_reference:503
             __m = (~__storage_type(0) << __result.__ctz_) & (~__storage_type(0) >> (__clz_r - __ddn));
-            *__result.__seg_ &= ~__m;
+            *__result.__seg_ = *__result.__seg_ & ~__m;
             if (__result.__ctz_ > __first.__ctz_)
----------------
Why is this required?


================
Comment at: libcxx/include/__bit_reference:958-961
+                if (__libcpp_is_constant_evaluated()) {
+                    for (size_t __i = 0; __i != __bit_array<_Cp>::_Np; ++__i)
+                        std::__construct_at(__b.__word_ + __i, 0);
+                }
----------------
Instead, I would begin the lifetime of the elements inside `__bit_array`'s constructor if we are during constant evaluation. That way you shouldn't need to change this code at all.


================
Comment at: libcxx/include/__memory/construct_at.h:43-44
 
+template <class _Tp, class ..._Args, class = decltype(::new (declval<void*>()) _Tp(declval<_Args>()...))>
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR
+_Tp* __construct_at(_Tp* __location, _Args&&... __args) {
----------------
Please match the style of the `class = decltype` above, just for consistency.


================
Comment at: libcxx/include/__memory/construct_at.h:49
+#else
+    return ::new (std::__voidify(*__location)) _Tp(std::forward<_Args>(__args)...);
+#endif
----------------
Let's add the `_LIBCPP_ASSERT` here too.


================
Comment at: libcxx/include/__utility/move.h:40-49
+template <class _Tp>
+_LIBCPP_NODISCARD_EXT inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR
+#ifdef _LIBCPP_NO_EXCEPTIONS
+typename remove_reference<_Tp>::type&&
+#else
+__move_if_noexcept_result_t<_Tp>
+#endif
----------------
I don't think this diff is needed in this patch, see my comment below.


================
Comment at: libcxx/include/memory:894
+    for (; __begin1 != __end1; ++__begin1, (void)++__begin2)
+        _Traits::construct(__a, std::__to_address(__begin2), std::__move_if_noexcept(*__begin1));
 }
----------------
I don't think we *need* to make this change to enable `constexpr` vector, right? If not, let's pull it into a separate patch, cause I'd like to have a discussion about it. Otherwise, can you please explain why it's needed?


================
Comment at: libcxx/include/memory:901
+_LIBCPP_CONSTEXPR_AFTER_CXX17 _LIBCPP_INLINE_VISIBILITY
 void __construct_forward_with_exception_guarantees(_Alloc&, _Tp* __begin1, _Tp* __end1, _Tp*& __begin2) {
+    __begin2 = std::__copy(__begin1, __end1, __begin2);
----------------
Given that `__begin2 = std::__copy(__begin1, __end1, __begin2);` does not compile, I think this overload is never used. Can you please confirm this by adding `static_assert(sizeof(_Alloc) == 0, "")` and running the tests? If the tests don't fail, then this is a dead code path and we'll need to investigate it.

**Edit** Well, it looks like we've got `enable_if<cond>::type` above as a template parameter. IOW, we're declaring a NTTP of type `void`, and that's never valid. 🤦🏼‍♂️ Clang should really tell us about that. Let's fix it in a separate patch prior to this one.


================
Comment at: libcxx/include/memory:902
 void __construct_forward_with_exception_guarantees(_Alloc&, _Tp* __begin1, _Tp* __end1, _Tp*& __begin2) {
-    ptrdiff_t _Np = __end1 - __begin1;
-    if (_Np > 0) {
-        _VSTD::memcpy(__begin2, __begin1, _Np * sizeof(_Tp));
-        __begin2 += _Np;
-    }
+    __begin2 = std::__copy(__begin1, __end1, __begin2);
 }
----------------
Why are we using `std::__copy` here? Let's use `std::copy` instead.


================
Comment at: libcxx/include/memory:923-928
+    if (__libcpp_is_constant_evaluated()) {
+        while (__begin1 != __end1)
+            std::__construct_at(__begin2++, *__begin1++);
+    } else {
+        __begin2 = std::__copy(__begin1, __end1, const_cast<_RawDest*>(__begin2)).second;
     }
----------------
I think this should all just be `uninitialized_copy` (or a `__` version of it).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68365