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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

LprettyGTM; just nits.



================
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)...);
+  }
----------------
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.)


================
Comment at: libcxx/include/__memory/ranges_construct_at.h:57
+inline constexpr auto construct_at = __construct_at::__fn(__function_like::__tag());
+
+} // namespace __cpo
----------------
You've got an extra blank line here (and line 79); and please compare to other CPO definitions to make sure you're consistent with the indentation.
I recommend looking on the right-hand side here: https://reviews.llvm.org/D115607#change-mUEJDUqE3FiU


================
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);
----------------
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.


================
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(...)
+                                          )>
----------------
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));
```


================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/specialized.construct/ranges_construct_at.pass.cpp:126
+  return 0;
+}
----------------
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));
```


================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/specialized.destroy/ranges_destroy_at.pass.cpp:51
+
+  friend void operator&(VirtualCountedBase) = delete;
+};
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116078