[libcxx-commits] [PATCH] D115626: [libc++][ranges] Implement `uninitialized_value_construct{, _n}` and `uninitialized_fill{, _n}`.

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const marked 2 inline comments as done.
var-const added a subscriber: Quuxplusone.
var-const added inline comments.


================
Comment at: libcxx/include/__memory/ranges_uninitialized_algorithms.h:46-47
     using _ValueType = remove_reference_t<iter_reference_t<_ForwardIterator>>;
-    return _VSTD::__uninitialized_default_construct<_ValueType>(__first, __last);
+    return _VSTD::__uninitialized_default_construct<_ValueType>(
+        _VSTD::move(__first), _VSTD::move(__last));
   }
----------------
ldionne wrote:
> How many columns do you have your Clang format configured to?
I'm actually not using `clang-format` yet (though I do intend to start using it), so this is formatted manually. I set it to 100 columns (FWIW, my personal preference is 80, but a lot of code I've seen uses 100, and quite a few declarations are by necessity verbose, making me lean towards 100).


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:100
+template <class _ForwardIterator, class _Tp>
+void uninitialized_fill(_ForwardIterator __first, _ForwardIterator __last, const _Tp& __x)
+{
----------------
ldionne wrote:
> Let's make those `_LIBCPP_HIDE_FROM_ABI`.
I marked them `_LIBCPP_INLINE_VISIBILITY` and `inline` for consistency with newer algorithms (e.g. `uninitialized_move`). Please let me know if either or both of these changes are wrong.

Regarding the visibility annotation -- I realize `_LIBCPP_INLINE_VISIBILITY ` is just an "alias" for `_LIBCPP_HIDE_FROM_ABI`. Which form is preferable here? The existing algorithms use `_LIBCPP_INLINE_VISIBILITY` (when they use an annotation).


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:150
     for (; __idx != __last; ++__idx)
-        ::new ((void*)_VSTD::addressof(*__idx)) _ValueType;
+        ::new (__voidify(*__idx)) _ValueType;
 #ifndef _LIBCPP_NO_EXCEPTIONS
----------------
ldionne wrote:
> Here and elsewhere.
Thanks!


================
Comment at: libcxx/include/__memory/voidify.h:25-29
+{
+  // Note: a C-style cast can cast away cv-qualifiers (it can be equivalent to
+  // doing a `static_cast` followed by a `const_cast`), thus achieving the
+  // same effect as the Standard mandates.
+  return (void*)_VSTD::addressof(__from);
----------------
ldionne wrote:
> Any reason why we don't literally copy-paste the definition from the Standard?
This is based on a previous (unfinished) discussion in [D115315](https://reviews.llvm.org/D115315). I think this form was advocated by @Quuxplusone. I didn't have a strong preference at the time of the discussion, but currently I'm leaning towards going with the Standard's definition.

My understanding is that the main advantage of using the C-style cast is its simplicity. However, it now seems to me that it's not so much simplicity as brevity, which isn't the same thing. The implementation from the Standard is, without doubt, clunky, however it a) explains exactly what's going on and b) draws attention to the fact that something unusual is going on. A C-style cast, however, can do one of several things, and remembering the rules and going through the exercise of applying them in this case is, I think, an extra burden for the reader; furthermore, given the C-style cast's brevity, the reader may be _unlikely_ to realize it's even an exercise worth going through because nothing really draws attention to it (other than "Why are we using a C-style cast?").

Finally, copy-pasting the Standard's definition is conceptually simpler. For the reader, there is no need to mentally verify that the two forms achieve exactly the same effect.

For these reasons, I'm currently leaning towards changing this to use the `static_cast` + `const_cast` combo. However, I would wait for @Quuxplusone's comments before making the change.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115626