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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM with my comments applied. Thanks!



================
Comment at: libcxx/include/__memory/construct_at.h:62-64
 template <class _ForwardIterator>
 _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17
 void destroy(_ForwardIterator, _ForwardIterator);
----------------
We don't need this anymore since we are using `__destroy` from the `is_array_v` overload of `destroy_at` below.


================
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));
   }
----------------
var-const wrote:
> 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).
Our `libcxx/.clang-format` uses 120 columns because indeed, we often have very verbose declarations.


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:100
+template <class _ForwardIterator, class _Tp>
+void uninitialized_fill(_ForwardIterator __first, _ForwardIterator __last, const _Tp& __x)
+{
----------------
var-const wrote:
> 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).
`_LIBCPP_HIDE_FROM_ABI` is the newer name for `_LIBCPP_INLINE_VISIBILITY`. I added an alias instead of renaming everything to `_LIBCPP_HIDE_FROM_ABI` because that would have created a lot of code churn.

I would say use `_LIBCPP_HIDE_FROM_ABI` for all new functions and functions that you are touching anyway.


================
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);
----------------
var-const wrote:
> 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.
> 
I +1 going with the standard's definition (and that's what I did when I first introduced `__voidify` before we removed it).


================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.fill/ranges_uninitialized_fill.pass.cpp:101-102
     std::destroy(buf.begin(), buf.end());
     assert(Counted::current_objects == 0);
     assert(Counted::total_objects == N);
     Counted::reset();
----------------
Since we are not testing `std::destroy` here, I would *not* add these assertions (here and in the tests for `uninitialized_default_construct`). That will make it more obvious that we only need to destroy them as "boilerplate" for testing the rest, but we're not actually trying to check anything related to `destroy` itself.

Can you pull out that refactoring and the other ones in the `.h` file for `uninitialized_default_construct` in its own patch? It should be trivial to approve but it'll simplify this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115626