[libcxx-commits] [PATCH] D116078: [libc++][ranges] Implement `construct_at` and `destroy{, _at}`.

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/__memory/ranges_construct_at.h:42
+
+  constexpr explicit __fn(__tag __x) noexcept : __function_like(__x) {}
+
----------------
Mordante wrote:
> I miss a lot of `_LIBCPP_HIDE_FROM_ABI` annotations.
Thanks for spotting this! It also affects my previous patches in this series (I'll do a fix patch once this series lands)


================
Comment at: libcxx/include/__memory/ranges_construct_at.h:48
+  constexpr _Tp* operator()(_Tp* __location, _Args&& ...__args) const {
+    return _VSTD::construct_at(__location, _VSTD::forward<_Args>(__args)...);
+  }
----------------
ldionne wrote:
> Quuxplusone wrote:
> > As always, I'd feel better if this were
> > ```
> > return ::new (_VSTD::__voidify(*__location)) _Tp(_VSTD::forward<_Args>(__args)...);
> > ```
> > (for obvious-correctness, simplicity of implementation, and efficiency-at-`-O0`) but I suppose the extra-indirection way has the minor benefit that if we ever need to do any special magic to make `construct_at` constexpr-friendly, we'll only need to do it in one place? (For better or worse, I guess. If it later turned out that that magic was //required// to happen only in `construct_at` and not in `ranges::construct_at`, then we'd have to go back to the simple `::new`-expression here anyway.)
> It's actually the other way around, if you use `::new (location)` then it won't be constexpr-friendly, right? `std::construct_at` was introduced exactly to make this sort of pattern constexpr friendly, unless I'm having a giant brain fart.
> 
> This seems to confirm what I'm saying: https://godbolt.org/z/9jb7jx4q3
> 
> TLDR: AFAICT, this is the only correct way to implement the function without having to change Clang (and possibly the other compilers we support).
Keeping as is given @ldionne's comment.


================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/specialized.construct/ranges_construct_at.pass.cpp:67-76
+  // Explicit multiargument constructor.
+  {
+    Foo f;
+
+    Foo* result = std::ranges::construct_at(&f, 42, 123.45, 'z');
+    assert(result == &f);
+    assert(f.x == 42);
----------------
Quuxplusone wrote:
> Let's make this
> ```
> struct Foo {
>     int a_;
>     int b_;
>     explicit Foo(int a, int b) : a_(b), b_(a) {}
>     Foo(std::initializer_list<int>);
>     void operator&() const = delete;
>     void operator,(auto&&) const = delete;
> };
> ~~~
> Foo f(0, 0);  // or auto f = Foo(0, 0);
> Foo *result = std::ranges::construct_at(std::addressof(f), 1, 2);
> assert(result == std::addressof(f));
> assert(result.a == 2);
> assert(result.b == 1);
> ```
> The major thing here is that I added `initializer_list` to make sure the library is correctly constructing with `Foo(1,2)` instead of `Foo{1,2}`. Then I threw in the deleted `&` and `,` overloads as an afterthought; I'm not really attached to them, but they seemed low-cost enough to justify their low benefit.
Done, thanks!


================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/specialized.construct/ranges_construct_at.pass.cpp:109-111
+// Make sure std::construct_at SFINAEs out based on the validity of calling the constructor, instead of hard-erroring.
+template <typename T, typename = decltype(std::ranges::construct_at((T*)nullptr, 1, 2) // missing arguments for Foo(...)
+                                          )>
----------------
Quuxplusone wrote:
> I smell clang-format's meddling in this weird comment-inside-the-parens style. ;)
> The comments are a bit confusing, especially coupled with the use of `nullptr` — we //do// check for `nullptr` inside `construct_at`, but that part is specifically //not// SFINAE-friendly. What I'd rather see here is something like
> ```
> constexpr bool can_construct_at(auto&&... args)
>   requires requires { std::ranges::construct_at(decltype(args)(args)...); }
>   { return true; }
> 
> constexpr bool can_construct_at(auto&&... args)
>   { return false; }
> 
> static_assert( can_construct_at((Foo*)nullptr, 1, 2));
> static_assert(!can_construct_at((Foo*)nullptr, 1));
> static_assert(!can_construct_at((Foo*)nullptr, 1, 2, 3));
> static_assert(!can_construct_at(nullptr, 1, 2));
> static_assert(!can_construct_at((int*)nullptr, 1, 2));
> static_assert(!can_construct_at(contiguous_iterator<Foo*>(), 1, 2));
> ```
Thanks! This definitely feels more comprehensive than the previous state. I've also backported this to the non-ranges test (from which I copied the previous state).


================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/specialized.construct/ranges_construct_at.pass.cpp:126
+  return 0;
+}
----------------
Quuxplusone wrote:
> I'd like to see some tests with array types, if that's supposed to work; or otherwise,
> ```
> static_assert(!can_construct_at((int(*)[])nullptr));
> static_assert(!can_construct_at((int(*)[2])nullptr));
> static_assert(!can_construct_at((int(*)[2])nullptr, 1, 2));
> 
> // and function types while we're at it
> static_assert(!can_construct_at((int(*)())nullptr));
> static_assert(!can_construct_at((int(*)())nullptr, nullptr));
> ```
Added the test for function types.

The situation with arrays is interesting. They don't work due to how the return type is currently specified (see https://godbolt.org/z/sT8xM7PvP), but there's no requirement that arrays should be SFINAE'd away. Also, there's a recently-updated proposal to support arrays: [LWG 3436](https://cplusplus.github.io/LWG/lwg-active.html#3436).

So we can a) make an extension to SFINAE away array types, then make it conditional in C++23 (or just remove it altogether) or b) do nothing and allow such calls to fail with a hard error. What do you think?


================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/specialized.destroy/ranges_destroy.pass.cpp:218
+  // TODO: Until std::construct_at has support for arrays, it's impossible to test this
+  //       in a constexpr context.
+  // static_assert(test_arrays());
----------------
Mordante wrote:
> Please add a link to D114903, which implements the missing feature.
Thanks for the link! Added to the new files and also to tests of the old versions of these algorithms.


================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/specialized.destroy/ranges_destroy_at.pass.cpp:51
+
+  friend void operator&(VirtualCountedBase) = delete;
+};
----------------
Quuxplusone wrote:
> Passing a polymorphic type by value should be a red flag. Let's make this
> `void operator&() const = delete;`
> and remove it from the derived class (because the derived class will inherit this one).
Changed here and in tests for the non-ranges version of `destroy_at`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116078