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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne 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);
----------------
var-const wrote:
> 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.
> 
We could restore the previous behavior by enclosing the array version of `__destroy_at` in `#if _LIBCPP_STD_VER > 17`.

I have a slight (but not strong) preference for that since it keeps us strictly conforming, and allows us to be lazy and not add a libc++ specific test for that "extension". I'll let you decide what to do here, but if you do support the extension, please add a test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115626