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

Wed Dec 15 18:52:03 PST 2021

var-const added inline comments.

================
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);
----------------
ldionne wrote:
> We don't need this anymore since we are using `__destroy` from the `is_array_v` overload of `destroy_at` below.
I had to replicate the "is array" SFINAE in the internal versions of `__destroy`, and unfortunately the forward declaration is now once again necessary.

The new change also makes it so that deleting an array of arrays now works in C++17 (previously it only worked in C++20):
```
int x[][2] = {{0, 1}, {2, 3}, {4, 5}};
std::destroy(x, x + 3); // Previously worked in C++20 mode but not in C++17 mode
// Now works in both modes.
```

>From the discussions I've seen on other patches, I don't think these functions should go out of their way to prevent usability improvements from later standards from seeping into older language modes (FWIW, in libstdc++ the code snippet works in C++17 mode). However, this is still something to call out, and please let me know if you think it's an issue.

================
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:
> 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.
(I personally prefer less columns because it makes it easier to see several files side-by-side)

Reformatted the files newly-added in this patch with `.clang-format`. I think I'll do an immediate follow-up once this patch lands to reformat `ranges_uninitialized_algorithms` with `clang-format` (to avoid introducing extra delta to this patch).

================
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:
> 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.
Done, thanks for the explanation.

================
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);
----------------
Quuxplusone wrote:
> ldionne wrote:
> > 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).
> I'm okay with reintroducing `__voidify` and copy-pasting from the Standard.
> (//Personally// I still think there was nothing wrong with `(void*)`, but if you're waiting for my feedback here, I'm saying "do what Louis says.")
Thanks! Changed the implementation of `__voidify` (and added a comment to explain the purpose of the casts).

================
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();
----------------
ldionne wrote:
> 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.
Removed from the code added in this patch, will create a separate patch for `uninitialized_default_construct`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115626