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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

Leaving comments but I won't mark as "needs changes" to avoid blocking you. If you implement my suggestions and get ✅ from other libc++ reviewers, 🚢 it.



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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116078